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.

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

Compile files and directories whose names start with . #1164

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

Conversation

mrm007
Copy link
Contributor

@mrm007 mrm007 commented May 7, 2023

Found this issue while testing #1163

Before merging:

@mrm007 mrm007 requested review from a team as code owners May 7, 2023 04:25
@changeset-bot
Copy link

changeset-bot bot commented May 7, 2023

🦋 Changeset detected

Latest commit: 3431c08

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
skuba Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment on lines 6 to 7
// 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

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:

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

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)

@72636c 72636c marked this pull request as draft July 14, 2023 00:09
@tadhglewis tadhglewis added the dino:snooze Snooze in Review Dino label Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dino:snooze Snooze in Review Dino
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants