Skip to content

Implement externalModuleIndicator #979

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

Merged
merged 16 commits into from
Jun 10, 2025
Merged

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented May 29, 2025

The infamous externalModuleIndicator field requires access to info like the module setting of the closest package.json in order to work in some situations. This is normally "fine"; just make a sidecar that the Program holds onto. But unfortunately, externalModuleIndicator is needed to know whether or not a given file needs to be reparsed to deal with top-level await, so needs to be there at parse time. This means that it's technically an input to parse, not something on the side.

Add this back in again, passing the metadata into parse.

This isn't great, and we have a plan for how to improve this later, but it does fix a bunch.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR migrates the externalModuleIndicator and related metadata onto the SourceFile during parse, removes the old sidecar structure, and wires metadata through the parser, host, and program layers.

  • Updated parser API signatures to accept compiler options and file metadata, and set both on each SourceFile in finishSourceFile.
  • Added getExternalModuleIndicator and helper functions to compute module indicators at parse time.
  • Refactored CompilerHost, Program, and file loader to pass metadata through parsing, and removed legacy metadata maps.

Reviewed Changes

Copilot reviewed 121 out of 121 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
internal/parser/parser.go Added options/metadata fields to Parser, set Metadata and ExternalModuleIndicator in finishSourceFile, and implemented getExternalModuleIndicator and helpers.
internal/parser/parser_test.go Updated benchmarks and fuzz tests to pass SourceFileAffectingCompilerOptions and metadata into ParseSourceFile.
internal/compiler/host.go Changed CompilerHost.GetSourceFile signature to accept metadata, and updated host implementation to panic if options are unset.
internal/compiler/program.go Wired metadata into NewProgram, UpdateProgram, added equalMetaData, and removed old metadata getters.
internal/compiler/fileloader.go Removed legacy metadata maps and parse tasks’ metadata, now loads metadata just before parsing each file.
Comments suppressed due to low confidence (2)

internal/parser/parser.go:114

  • ParseJSONText does not set the new Metadata or ExternalModuleIndicator fields on its SourceFile, so JSON inputs will lack module detection metadata. Consider invoking finishSourceFile or manually assigning result.Metadata and result.ExternalModuleIndicator before returning.
return result

internal/parser/parser.go:101

  • [nitpick] The parameter name metadata shadows the parser's internal metadata field and may be confused with other metadata. Rename this parameter (e.g., to fileMetadata) for clarity.
func ParseSourceFile(fileName string, path tspath.Path, sourceText string, options *core.SourceFileAffectingCompilerOptions, metadata *ast.SourceFileMetaData, jsdocParsingMode scanner.JSDocParsingMode) *ast.SourceFile {

@andrewbranch
Copy link
Member

I need to look in more detail later, but my immediate thought is that the cache key could have the pre-computed value of “force this file to be a module” instead of all the compiler options that might contribute to that value. Right now, differing moduleResolution or jsx settings will prevent source file reuse, but that needn’t be true if both projects have --moduleDetection force.

@jakebailey
Copy link
Member Author

That was my thought too, but the trouble is that the indicator is an AST node, because we need to be able to point diagnostics at it in some situations. But, for these new cases they're basically all just what used to be "true", so potentially that would work as a plain boolean passed in.

I'm just not sure that we'll actually end up in the situation where that reuse would have happened, though.

@andrewbranch
Copy link
Member

I think it’s pretty common for monorepos to have projects where these settings differ, but the actual parse doesn’t. It seems silly to not be able to share common declaration files between those projects, when the declaration files themselves are never affected by moduleDetection in the first place.

@jakebailey jakebailey marked this pull request as draft June 3, 2025 04:40
@jakebailey
Copy link
Member Author

This PR is pretty broken now; I'm going to redo it, not eliminate the metadata sidecar and instead still pass stuff into the parser as an input, which should produce the smallest diff.

We discussed a new way to do reparsing, however that's a much more difficult undertaking, not just because it means redoing things, but also because reparsing happening in the parser during the initial parse means that the Parser struct is still there and populated with stuff like the identifier list and so on (maybe other important state), so it needs a little bit of thinking.

@@ -1068,7 +1070,7 @@ func (p *Project) Close() {

if p.program != nil {
for _, sourceFile := range p.program.GetSourceFiles() {
p.host.DocumentRegistry().ReleaseDocument(sourceFile, p.program.Options())
p.host.DocumentRegistry().ReleaseDocument(sourceFile, p.program.Options(), p.program.GetSourceFileMetaData(sourceFile.Path()))
Copy link
Member Author

Choose a reason for hiding this comment

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

I really don't like how much jumping through hoops I have to do for this.

Honestly, it seems to me like the the entire set of parameters to ParseSourceFile are the key, and that we could store a copy on the source files, rather than constantly asking the program for them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course, that would not work out for that differing parse plan.

Copy link
Member

Choose a reason for hiding this comment

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

It's correct that the key and the parse params are the same and storing a copy/pointer on the file makes sense. It's just that we may want to try to trim down the key/params later, with SourceFileMetadata in particular containing some stuff we might want to pull off. But I think it's fine to make a type that is the cache key and store it on the file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I make that change now? Would theoretically simplify things, though at that point I would find it strange that we'd also have the sidecar and plumb that access through everywhere.

Copy link
Member

Choose a reason for hiding this comment

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

No, I might see if I can follow up with a way to just pass in the module status, so let’s do that first

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the change to make the above work is extensive, I spent a while trying to make it work and it exposed a bunch of other weirdness. I'll send a different PR for that.

@jakebailey jakebailey marked this pull request as ready for review June 9, 2025 18:22
@jakebailey jakebailey added this pull request to the merge queue Jun 10, 2025
Merged via the queue into main with commit 9cf6764 Jun 10, 2025
23 checks passed
@jakebailey jakebailey deleted the jabaile/external-module-indicator branch June 10, 2025 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants