handle multi-line comments in AssetMapper javascript compiler #58944
Conversation
|
Hey! Thanks for your PR. You are targeting branch "7.3" but it seems your PR description refers to branch "6.4". Cheers! Carsonbot |
|
drat, how do I change the target branch? |
Press Edit next to the title, it allows to edit the title and the target branch. |
| yield 'import_in_multiline_comment' => [ | ||
| 'input' => <<<EOF | ||
| // Asynchronous loading | ||
| /** | ||
| * Used in conjunction with the `m` component to reduce bundle size. | ||
| * | ||
| * `m` is a version of the `motion` component that only loads functionality | ||
| * critical for the initial render. | ||
| * | ||
| * `LazyMotion` can then be used to either synchronously or asynchronously | ||
| * load animation and gesture support. | ||
| * | ||
| * ```jsx | ||
| * // Synchronous loading | ||
| * import { LazyMotion, m, domAnimations } from "framer-motion" | ||
| * | ||
| * function App() { | ||
| * return ( | ||
| * <LazyMotion features={domAnimations}> | ||
| * <m.div animate={{ scale: 2 }} /> | ||
| * </LazyMotion> | ||
| * ) | ||
| * } | ||
| * | ||
| * // Asynchronous loading | ||
| * import { LazyMotion, m } from "framer-motion" | ||
| * | ||
| * function App() { | ||
| * return ( | ||
| * <LazyMotion features={() => import('./path/to/domAnimations')}> | ||
| * <m.div animate={{ scale: 2 }} /> | ||
| * </LazyMotion> | ||
| * ) | ||
| * } | ||
| * ``` | ||
| * | ||
| * @public | ||
| */ | ||
| EOF, | ||
| 'expectedJavaScriptImports' => [], | ||
| ]; | ||
|
|
There was a problem hiding this comment.
Nice catch, @tacman!
Could you reduce this test to the minimal amount of code? It will be easier to later maintain and also will ensure we are testing this case and not any other conditions. :)
|
@tacman you should not merge branch 7.2 into your feature branch (and even less if you submit this to 6.4) |
|
ack! I was trying to fix the failed tests, and merged the wrong branch. git revert HEAD isn't working, is there a way I can simply delete my last commit (which was erroneously merging 7.2)? |
|
@tacman you need to reset your branch (not revert, which creates more commits) and perform a force push. |
903d00c to
ca4a779
Compare
|
OK, hopefully my branch and PR are okay now. |
| @@ -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) { | |||
There was a problem hiding this comment.
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.
|
alas, I'm not sure how to solve this then. I imagine with some sort of multi-line comment regex. On a related note, I'm surprised that api-platform hasn't minified this file in such a way to remove the comments. Of course, it wouldn't be a real solution to this problem, but avoid the immediate issue integrating it. |
| yield 'import_in_multiline_comment' => [ | ||
| 'input' => <<<EOF | ||
| /** | ||
| * <LazyMotion features={() => import('./path/to/domAnimations')} /> | ||
| * ``` | ||
| */ | ||
| EOF, | ||
| 'expectedJavaScriptImports' => [], | ||
| ]; | ||
|
|
There was a problem hiding this comment.
This test is already passing on 6.4 and 7.1 (on my machine).. Could you double-check ?
There was a problem hiding this comment.
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=/apiThere was a problem hiding this comment.
I'm just trying to get a "before" / "after" here ;) Without it i cannot be sure if what i'm doing has any effect :)
|
I think this is due/new because of this change in the recipes: symfony/recipes@ab0209c You can hide this by disabling the strict check in dev For the parsing part, i made a PR some time ago to add some "parser" code and improve the detection of import.. It seems it's time to reopen it :) |
|
+1 for the ad-hoc parser PR. I don't think we'll manage to fix this reliably just with regular expressions. |
|
It seems to me that the strict setting for 3rd-party bundles is problematic. It's like the deprecation warnings the fill the logs when it's out of the developers control. Is it possible to make the missing_import_mode related only to the developer's code and excludes /vendor? |
Reopen of symfony#54134 after symfony#58944
|
Hi, Maybe you could just remove comments first? #src/Symfony/Component/AssetMapper/Compiler/JavaScriptImportPathCompiler.phpL62
public function compile(string $content, MappedAsset $asset, AssetMapperInterface $assetMapper): string
{
$pattern = '/(?:(?:\/\*(?:[^*]|(?:\*+[^*\/]))*\*+\/)|(?:(?<!\:|\\\|\'|\")\/\/.*))/';
$content = preg_replace($pattern, '', $content);
return preg_replace_callback(self::IMPORT_PATTERN, ... |
We do not want to change the source files here (except to replace the url with the good ones). |
|
The only way to perfectly ignore comments is to use a state-machine / parser (see #59004) |
…nandre) This PR was squashed before being merged into the 7.3 branch. Discussion ---------- [AssetMapper] Detect import with a sequence parser | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #58928 and #58944 (maybe) | License | MIT Third attempt after #54134 and #58999.. this is the good one 👍 I initially started this PR to learn / try things, but it really behaves well.. and _may_ stop (or help stopping) what seems to be a never-ending suite of special cases. Open to any feedback Commits ------- 720c387 [AssetMapper] Detect import with a sequence parser
This fixes the issue where assetmapper tries to include a javascript import that is embedded in a comment.