-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Add moduleDetection compiler flag to allow for changing how modules are parsed #47495
Conversation
…re parsed The default setting is 'auto', where JSX containing files under react-jsx and react-jsxdev are always parsed as modules, and esm-format files under module: node12+ are always parsed as modules, in addition to the 'legacy' detection mode's conditions for other files. (Declaration files are exempt from these new conditions) The 'legacy' mode preserves TS's behavior prior to the introduction of this flag - a file is parsed as a module if it contains an import, export, or import.meta expression. In addition, there is a 'force' mode that forces all non-declaration files to be parsed as modules. (Declaration files are still only modules if they contain a top-level import or export.) This technically breaks the parser API, but it's kinda-sorta backwards compatible so long as you don't need the functionality associated with more recent compiler flags.
Can you add an explanation to the PR description of why 'legacy' is needed -- basically, what you expect to break if everybody were upgraded to 'auto' with no escape hatch. |
Hm, I alluded to it, but it would break people with global script files using |
Does this include |
Yep. |
* The callback used to set the external module indicator - this is saved to | ||
* be later reused during incremental reparsing, which otherwise lacks the information | ||
* to set this field | ||
*/ | ||
/* @internal */ setExternalModuleIndicator?: (file: SourceFile) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which otherwise lacks the information to set this field
Why is this the case now but not before this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the module indicator now depends on the compiler options the source file was created with, which the source file retains no direct reference to, and small incremental edits (the kind that don't do a full source file rebuild) don't pass in full options, either, they just reuse the old source file's data - so we have to keep the callback around to reuse in that incremental scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I forgot that parsing in general, much less incremental parsing, doesn’t have access to CompilerOptions; only the (previously very small) subset of them that it needs. I’m a little worried that this will become a footgun whereby we or other API consumers will accidentally close over and retain something they shouldn’t in this function (it you haven’t seen the implementation, you don’t know a reference to it is stored on the SourceFile—the property is even erased from the public API!). What if CreateSourceFileOptions
took the moduleDetection
, module
, and target
from CompilerOptions and stored those, such that the function currently returned by getSetExternalModuleIndicator
would have everything it needs to run on the SourceFile after creation? It would be more properties to store, but more straightforward. It took me a minute to understand the purpose of this indirection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, while that would be enough for recalculating the correct closure, it doesn't do much to help the impliedNodeFormat
field calculation, which needs extensive host availability anyway. I'm disinclined to make these look like CompilerOptions
because of that - if you just passed in compiler options, you'd be broken in any nodenext type scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
impliedNodeFormat
now gets passed into createSourceFile
(and I wasn’t proposing removing that, for the reason you mentioned), and I was assuming it can’t change during updateSourceFile
(incremental parsing)—is that right? If so, then incremental parsing doesn’t need to worry about impliedNodeFormat
.
I’m unclear on what constitutes an “esm-format file” if it’s not based on the presence of import/export, seeing as ESM mode is controlled by the runtime, not the file contents. Does this mean node.js semantics, i.e. If the above is the case it sounds like I probably want to use |
Yes and yes. Esm mode modules are the only ones which transpile to esm output, to whit (cjs mode files transpile to cjs). Honestly, if you're targeting browser modules you probably want |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good idea. I made some low-level comments, but I don't understand the higher-level structure enough to make useful comments there.
I'll look at the tests next.
/** | ||
* This is a somewhat unavoidable full tree walk to locate a JSX tag - `import.meta` requires the same, | ||
* but we avoid that walk (or parts of it) if at all possible using the `PossiblyContainsImportMeta` node flag. | ||
* Unfortunately, there's no `NodeFlag` space to do the same for JSX. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should there be? (there might not be any NodeFlags left in any case)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bails on the first JSX tag you find in a JSX tag, which is usually almost instantly, so.... probably not? Still, I think node.transformFlags & TransformFlags.ContainsJsx
probably already tracks the same thing and post-node-factory change should always be set, so... I'll try that out for narrowing the search.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done looking at the tests, just waiting on answers to earlier questions.
@weswigham Does having a |
A |
Ah okay, the script mode stuff doesn't refer to cjs but old-fashioned browser scripts. Thanks. |
The default setting is
'auto'
, where JSX containing files under react-jsx and react-jsxdev arealways parsed as modules, and esm-format files under module: node12+ are always parsed as modules,
in addition to the 'legacy' detection mode's conditions for other files. (Declaration files are exempt from
these new conditions)
The
'legacy'
mode preserves TS's behavior prior to the introduction of this flag - a file isparsed as a module if it contains an import, export, or import.meta expression.
In addition, there is a
'force'
mode that forces all non-declaration files to be parsed as modules.(Declaration files are still only modules if they contain a top-level import or export.)
This technically breaks the parser API, but it's kinda-sorta backwards compatible so long
as you don't need the functionality associated with more recent compiler flags.
While
"auto"
is a mostly backwards compatible new default (technically it breaksreact-jsx
output, but it does so in a case where the output was nonfunctional and fixes it, so it's fine), most people probably just want to use"force"
unless they intend to have script files in their compilation."moduleDetection": "auto"
(the new default):Fixes #46723
Fixes #40501
Fixes #47237
"moduleDetection": "force"
:Fixes #18232
Fixes #14279