Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Compile files and directories whose names start with . #1164

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Compile files and directories whose names start with . #1164

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0ab85ff
Compile files and directories whose names start with .
mrm007 May 7, 2023
2c96e5b
do the crawling for TypeScript files ourselves, then feed the list to…
mrm007 May 11, 2023
ceacf25
update test snapshot
mrm007 May 11, 2023
3431c08
Merge branch 'master' into vocab-dirs
72636c May 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fifty-cougars-begin.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'skuba': patch
---

build: When using esbuild, ensure files and directories whose names start with . are also compiled
9 changes: 8 additions & 1 deletion src/cli/configure/analysis/__snapshots__/project.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,14 @@ export {};
"data": "{
"exclude": ["**/__mocks__/**/*", "**/*.test.ts", "src/testing/**/*"],
"extends": "./tsconfig.json",
"include": ["src/**/*"]
"include": [
"src/**/*",
// Must be explicit about including dotfiles otherwise TypeScript ignores them
// https://github.com/microsoft/TypeScript/blob/v5.0.4/src/compiler/utilities.ts#L8828-L8830
"src/**/.*",
"src/**/.*/**/*",
"src/**/.*/**/.*"
]
}
",
"operation": "A",
Expand Down
9 changes: 8 additions & 1 deletion template/base/tsconfig.build.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
{
"exclude": ["**/__mocks__/**/*", "**/*.test.ts", "src/testing/**/*"],
"extends": "./tsconfig.json",
"include": ["src/**/*"]
"include": [
"src/**/*",
// Must be explicit about including dotfiles otherwise TypeScript ignores them
// https://github.com/microsoft/TypeScript/blob/v5.0.4/src/compiler/utilities.ts#L8828-L8830
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finding this @mrm007. I recall you first mentioned that this affected our esbuild implementation; does it affect tsc too?

If it's esbuild only then I'm hoping we can add some adapter logic to deal with this under the hood.

Copy link
Contributor Author

@mrm007 mrm007 May 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strangely, it's only for esbuild. Running tsc via the command line finds and compiles all files, as expected.

The filter is applied only when resolving the config programmatically.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay wow, this is a great find. It comes down to this and is actually a deeper bug with the esbuild implementation:

skuba/src/cli/build/esbuild.ts

Lines 68 to 82 in 3196eb1

const parsedCommandLine = ts.parseJsonConfigFileContent(
readConfigFile.config,
ts.sys,
tscArgs.dirname,
);
if (parsedCommandLine.errors.length) {
log.err(`Could not parse ${tscArgs.pathname}.`);
log.subtle(ts.formatDiagnostics(parsedCommandLine.errors, formatHost));
process.exitCode = 1;
return;
}
const { fileNames: entryPoints, options: compilerOptions } =
parsedCommandLine;

The TypeScript API does not include dot paths when deriving fileNames and this holds true for tsc internally. However, tsc performs module resolution and will subsequently include files like .vocab/index.ts only if they have been imported by another module in fileNames. On the other hand, our esbuild adapter disables bundling and ends up classifying files like .vocab/index.ts as "external" dependencies that should not be included.

There are a few options here:

  1. Ask consumers to manually modify their tsconfig.json#include as per this PR. This is simple but feels like a rather arcane fix.
  2. Use a custom method to derive fileNames in the esbuild adapter that includes dot paths by default. This would let us play nicely with Vocab but won't promise to match the TypeScript implementation.
  3. We can read the metafile out of esbuild and identify imports that should be included in a second pass.

Copy link
Contributor Author

@mrm007 mrm007 May 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we do the globbing ourselves and include all the dotfiles? Basically, applying the same technique mentioned here #1163 (comment):

  • take the root directory from skuba.entryPoint i.e. src for src/index.ts
  • find all *.{ts,tsx,cts,mts} files
  • compile them with esbuild

Something like this 2c96e5b (#1164)

"src/**/.*",
"src/**/.*/**/*",
"src/**/.*/**/.*"
]
}