- Notifications
You must be signed in to change notification settings - Fork664
Handle synthetic tslib and JSX factory imports#780
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| @@ -0,0 +1,44 @@ | |||
| --- old.commentsOnJSXExpressionsArePreserved(jsx=react-jsx,module=commonjs,moduledetection=auto).errors.txt | |||
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 is broken because we do not correctly set the module indicators.
863c22d toecc1236Compare| ->notAHelper : Symbol(notAHelper, Decl(tslib.d.ts, --, --)) | ||
| - | ||
| +>notAHelper : Symbol(notAHelper, Decl(tslib.d.ts, 0, 12)) |
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.
Funny, Strada's regex here is bad, while in Corsa we "fixed" this.
Future TODO: stop using synthetic nodes for this, because we only use them to then plug into module resolution anyway. There's no real reason to use a Node here other than "the code used to work this way and it's not very trivial to change", but changing it will be faster and make the loader lock-free again. |
a88fef7Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Fixes#765
Fixes#767
This implements import helper and JSX factory import node synthesis and resolution. Since the AST node is immutable, we cannot modify
SourceFile.Imports. This info is now stored on theProgramrather than at special fixed offsets inimportsthat's mutated after parse.This could allow us to reuse ASTs more often, and means we stop having to work around these synthesized imports in other parts of the code (LS features, mainly).
This PR isn't perfect; it turns out we're missing implementations of
moduleDetectionandexternalModuleIndicator, so we're not correctly determining that JSX files are modules because thejsxsetting plays a part in determining if a file is a module (yay).Fixing that will have to be a follow-up PR but that's going to be a lot more invasive. This PR fixes most stuff as it is now.