-
Notifications
You must be signed in to change notification settings - Fork 13.2k
Fix handling of templates in preProcessFile #36143
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
Conversation
|
Thanks for the PR! It looks like you've changed 'preProcess.ts' in some way. Please ensure that any changes here don't break consumers with unique project systems such as Visual Studio. Pinging @sheetalkamat, @amcasey, and @minestarks so they are aware of the changes. |
|
CI failure looks unrelated. Closing and re-opening to retrigger. |
| currentToken = scanner.reScanTemplateToken(); | ||
| } | ||
|
|
||
| if (currentToken === SyntaxKind.TemplateHead) { |
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 technically needs to track more than one scanning context. For example,
`${(<div>This is JSX text so these: `` ${} mean nothing, and in fact, the {} are
(empty) interpolation areas! So ${import("mod")} and {import("mod")} are dynamic imports! Also,
you could have ${/*A comment*/} or ${/*import("ignored")*/} a commented module </div>)}`;The textmate grammar chokes on that one, too, since it involves multiple nested scanning contexts, but it is parsed correctly.
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 am a bit unfamiliar with how this works, but I will add these test cases and see if I can figure it out. Thanks!
|
Seems like a superset of #33688, or maybe that PR should be folded into this PR. |
|
@lenkan Do you need any help? Do you know if you'll get back to this? |
|
@weswigham - I constructed some test cases based on the examples you gave in #36143 (comment). But I couldn't patch the current solution to support those. I have struggled to find the time to dig deeper into it. If you can give me any pointers towards what I need to consider in the implementation that would be appreciated. I feel that the solution direction I took is not very robust, but I lack enough knowledge of this project to come up with something different at this stage. Meanwhile, I've rebased this branch and resolved the conflicts in the test file. |
|
Rather than a single |
weswigham
left a comment
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.
Requesting the changes mentioned above~
|
@lenkan Do you want to keep this open? Let us know if we can help with more, detailed explanation. |
|
@sandersn I will close it because I won't have time to figure out how to change this behaviour without breaking something else in |
Previously, the preProcessFile function would erroneously identify import statements inside template literals if they occurred after a template literal that contained a placeholder.
Would report
m1as being an imported file.Inversely, it would fail to find valid imports in the similar situation:
This is because the code is failing to recognize the `}`` as the end of the first template literal.
This PR fixes that by keeping track of if we are inside a template, if so, we run the "rescanTemplateToken" when we encounter a close brace.
Fixes #30878