Skip to content

handle multi-line comments in AssetMapper javascript compiler #58944

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

handle multi-line comments in AssetMapper javascript compiler #58944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7aa89ac
handle multi-line comments in AssetMapper javscript compiler
tacman Nov 20, 2024
ca4a779
reduce size ImportPath with comments test
tacman Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/Symfony/Component/AssetMapper/Compiler/JavaScriptImportPathCompiler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,11 @@ private function isCommentedOut(mixed $offsetStart, string $fullContent): bool
return true;
}

// inside a multi-line comment, since javascript can't start with a '* '
if ('* ' === $firstTwoChars) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is broken for multiple reasons:

  • a JS line can actually start with * if you split a long expression at the position of a multiplication operator, which is totally valid
  • lines inside a multiline comment are not required to start with * . JSDoc comments follow such convention, but as far as the JS parser is concerned, the * is just part of the comment text.

Anyway, by definition of a multiline comment, you cannot identify that you are inside such a comment by inspecting only the currently line.

return true;
}

if ('/*' === $firstTwoChars) {
$commentEnd = strpos($fullContent, '*/', $lineStart);
// if we can't find the end comment, be cautious: assume this is not a comment
Expand Down
10 changes: 10 additions & 0 deletions src/Symfony/Component/AssetMapper/Tests/Compiler/JavaScriptImportPathCompilerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,16 @@ public function testCompileFindsCorrectImports(string $input, array $expectedJav

public static function provideCompileTests(): iterable
{
yield 'import_in_multiline_comment' => [
'input' => <<<EOF
/**
* <LazyMotion features={() => import('./path/to/domAnimations')} />
* ```
*/
EOF,
'expectedJavaScriptImports' => [],
];

Comment on lines +96 to +105
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is already passing on 6.4 and 7.1 (on my machine).. Could you double-check ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then perhaps the test is wrong. The underlying problem is with an API Platform js file, causing this trivial example to fail.

symfony new MyApp  --webapp && cd MyApp
composer req api
bin/console make:entity -a Task -n
symfony server:start -d
symfony open:local --path=/api

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just trying to get a "before" / "after" here ;) Without it i cannot be sure if what i'm doing has any effect :)

yield 'standard_symfony_app_js' => [
'input' => <<<EOF
import './bootstrap.js';
Expand Down
Loading