fix: handle faulty imports

[caalador]

Do not throw silently for incompatible
import paths. Fix comment removal
for JS files to accept ' as string.

fixes #12765

[github-actions]

Unit Test Results

   774 files  +  4     774 suites  +4   29m 45s ⏱️ + 4m 32s
5 769 tests +25  5 720 ✔️ +27  49 💤  - 2  0 ❌ ±0 
5 793 runs  +17  5 744 ✔️ +20  49 💤  - 3  0 ❌ ±0 

Results for commit dbb1783. ± Comparison against base commit eaad21e.

♻️ This comment has been updated with latest results.

[joheriks]

The original StringUtilTest::removeComments is used only here and in Lit and Polymer template parsers. Is there a reason that removeJsComments couldn't replace the previous removeComments (which would also remove the code duplication)?

[caalador]

The main reason is that it is used to clean the return html'....' html contents and with the new js version it would see the whole html as sting and not remove comments from there making it not work as wanted.

[joheriks]

The main reason is that it is used to clean the return html'....' html contents and with the new js version it would see the whole html as sting and not remove comments from there making it not work as wanted.

Is html'...' actually legal JS, doesn't this notation require backticks (html`...` ) so it should not be affected? If there is a way to quote the template with apostrophes, then I would suggest to parametrize the parser instead of code duplication, as the only difference between removeComments and removeJsComments is whether to transition to the IN_STRING_APOSTROPHE state.

[caalador]

The main reason is that it is used to clean the return html'....' html contents and with the new js version it would see the whole html as sting and not remove comments from there making it not work as wanted.

Is html'...' actually legal JS, doesn't this notation require backticks (html`...`) so it should not be affected? If there is a way to quote the template with apostrophes, then I would suggest to parametrize the parser instead of code duplication, as the only difference between removeComments and removeJsComments is whether to transition to the IN_STRING_APOSTROPHE state.

Well it is for polymer template and there it is actually fine to have it with `|'|". The original tests were at least testing that we can remove the comments from the template method.

[joheriks]

The main reason is that it is used to clean the return html'....' html contents and with the new js version it would see the whole html as sting and not remove comments from there making it not work as wanted.

Is html'...' actually legal JS, doesn't this notation require backticks (html`...`) so it should not be affected? If there is a way to quote the template with apostrophes, then I would suggest to parametrize the parser instead of code duplication, as the only difference between removeComments and removeJsComments is whether to transition to the IN_STRING_APOSTROPHE state.

Well it is for polymer template and there it is actually fine to have it with `|'|". The original tests were at least testing that we can remove the comments from the template method.

Aight, my suggestion would be to refactor the method to reduce the code duplication and mention in the JavaDoc why the parameter is required (different parsing requirements for polymer templates).

[caalador]

Done.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

[vaadin-bot]

Hi @caalador and @joheriks, when i performed cherry-pick to this commit to 2.8, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 9d4af71
error: could not apply 9d4af71... fix: handle faulty imports (#12793)
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add ' or 'git rm '
hint: and commit the result with 'git commit'

[vaadin-bot]

Hi @caalador and @joheriks, when i performed cherry-pick to this commit to 9.0, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 9d4af71
error: could not apply 9d4af71... fix: handle faulty imports (#12793)
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add ' or 'git rm '
hint: and commit the result with 'git commit'

[vaadin-bot]

Hi @caalador and @joheriks, when i performed cherry-pick to this commit to 2.7, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 9d4af71
error: could not apply 9d4af71... fix: handle faulty imports (#12793)
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add ' or 'git rm '
hint: and commit the result with 'git commit'

[vaadin-bot]

This ticket/PR has been released with platform 23.0.0.beta1 and is also targeting the upcoming stable 23.0.0 version.