Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[AssetMapper] Fix JavaScript compiler load imports from JS strings#53652
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
[AssetMapper] Fix JavaScript compiler load imports from JS strings#53652
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedJan 27, 2024
Hey! Thanks for your PR. You are targeting branch "7.1" but it seems your PR description refers to branch "6.4". Cheers! Carsonbot |
| $lineStart =strrpos($fullContent,"\n",$offsetStart -\strlen($fullContent)); | ||
| // Remove enclosed strings before the import | ||
| $lineContentBeforeImport =substr($fullContent,$lineStart,$offsetStart -$lineStart); | ||
| $lineWithoutStrings =preg_replace('/\'(?:[^\'\\\\]|\\\\.)*\'|"(?:[^"\\\\]|\\\\.)*"/','',$lineContentBeforeImport); |
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.
template literals (`) are quite common in modern js, and this syntax allows multiline strings making this check quite a challenge
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.
@kaznovac this PR fixes a bug with console log / deprecation / help messages (cf the original issue)
Handling multi-line strings/comments can reveal itself really complex, and probably would need there some concrete JS parser.
I must admit i doubt any library would use multi-line template comments in source code containing import instructions.. but if/when that happen we could handle it in a later PR ?
WDYT ?
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.
alternative implementation idea: we merge this regexp with the one that checks for imports.
Since regexp are greedy, this will make the filter in one go, with just a quick check at the start of the closure
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
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.
@nicolas-grekas got it, and it seems to work perfectly ! Thank you :)
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.
@smnandre great work && sorry for the late answer
p.s. please note that this method can now be removed
src/Symfony/Component/AssetMapper/Compiler/JavaScriptImportPathCompiler.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
b8aa830 tod70e500Comparenicolas-grekas commentedFeb 1, 2024
Thank you@smnandre. |
Add a check to ensure the import is not parsed from a JS string (e.g.
console.log(...)in the issue)