Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[AssetMapper] Ignore comment lines in JavaScriptImportPathCompiler#53893
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
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.
Very complex regex, as expected :). We’re living by the tests on this by necessity. Thanks for fixing this!
| * | ||
| * @see https://regex101.com/r/1iBAIb/1 | ||
| */ | ||
| private const IMPORT_PATTERN = '/^(?:\/\/.*)|(?:\'(?:[^\'\\\\]|\\\\.)*\'|"(?:[^"\\\\]|\\\\.)*")|(?:import\s*(?:(?:\*\s*as\s+\w+|\s+[\w\s{},*]+)\s*from\s*)?|\bimport\()\s*[\'"`](\.\/[^\'"`]+|(\.\.\/)*[^\'"`]+)[\'"`]\s*[;\)]?/m'; |
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.
We can probably inline the comments directly in the regex (and add some spaces/new lines) to make the regex more easily understandable by using/ms.
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.
Is that better... or a bit too much ?
privateconstIMPORT_PATTERN ='/ ^ (?:\/\/.*) # Lines that start with comments | (?:\'(?:[^\'\\\\]|\\\\.)*\' # Strings enclosed in single quotes | "(?:[^"\\\\]|\\\\.)*" # Strings enclosed in double quotes ) | (?: # Import statements (script captured) import\s* (?: (?:\*\s*as\s+\w+|\s+[\w\s{},*]+) \s*from\s* )? | \bimport\( ) \s*[\'"`](\.\/[^\'"`]+|(\.\.\/)*[^\'"`]+)[\'"`]\s*[;\)] ?/mx';
AurelienPillevesseFeb 10, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
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.
Another solution can be :
privateconstPATTERN_LINES_START_WITH_COMMENTS ='(?:\/\/.*)';privateconstPATTERN_STRINGS_ENCLOSED_SINGLE_OR_DOUBLE_QUOTES ='(?:\'(?:[^\'\\\\]|\\\\.)*\' # Strings enclosed in single quotes | "(?:[^"\\\\]|\\\\.)*" # Strings enclosed in double quotes )';// ...
And you concat everything in theIMPORT_PATTERN. What do you think about it ?
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 genuinely have no opinion here, just happy if the next person to work with this pattern does not lose time to understand why and how we made this regexp :) So you guys tell me
Thank you@smnandre. |
4927b8e to41a3489Compare
The following code was wrongly treated asone line of comment
This PR updates the regexp to ignore comment lines (andfix#53849)
Also added a comment explaining how the regexp works for future maintenance.