Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

only the first line of comments before import statement is preserved #4953

Closed
viocha opened this issue Apr 23, 2023 · 6 comments · Fixed by #4975
Closed

only the first line of comments before import statement is preserved #4953

viocha opened this issue Apr 23, 2023 · 6 comments · Fixed by #4975

Comments

@viocha
Copy link

viocha commented Apr 23, 2023

Rollup Version

3.20.7

Operating System (or Browser)

windows

Node Version (if applicable)

No response

Link To Reproduction

https://rollupjs.org/repl/?version=3.20.7&shareable=JTdCJTIyZXhhbXBsZSUyMiUzQW51bGwlMkMlMjJtb2R1bGVzJTIyJTNBJTVCJTdCJTIyY29kZSUyMiUzQSUyMiUyRiUyRiUyMCUzRCUzRFVzZXJTY3JpcHQlM0QlM0QlNUNuJTJGJTJGJTIwJTQwbmFtZSUyMCUyMCUyMCUyMCUyMCUyMCUyMCUyME5ldyUyMERlbW8lNUNuJTJGJTJGJTIwJTQwbmFtZXNwYWNlJTIwJTIwJTIwVmlvbGVudG1vbmtleSUyMFNjcmlwdHMlNUNuJTJGJTJGJTIwJTQwbWF0Y2glMjAlMjAlMjAlMjAlMjAlMjAlMjAlM0NhbGxfdXJscyUzRSU1Q24lMkYlMkYlMjAlNDByZXF1aXJlJTIwJTIwJTIwJTIwJTIwaHR0cHMlM0ElMkYlMkZjZG4uanNkZWxpdnIubmV0JTJGbnBtJTJGanF1ZXJ5JTQwMyU1Q24lMkYlMkYlMjAlNDB2ZXJzaW9uJTIwJTIwJTIwJTIwJTIwMS4wJTVDbiUyRiUyRiUyMCUzRCUzRCUyRlVzZXJTY3JpcHQlM0QlM0QlNUNuJTVDbiUyRiUyRiUyMGNvbW1lbnRzJTIwYmVmb3JlJTIwaW1wb3J0JTIwc3RhdGVtZW50JTVDbmltcG9ydCUyMGRhdGElMjBmcm9tJTIwJy4lMkZhLmpzJyUzQiU1Q24lNUNuJTVDbmNvbnNvbGUubG9nKGRhdGEpJTNCJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlMkMlMjJuYW1lJTIyJTNBJTIybWFpbi5qcyUyMiU3RCUyQyU3QiUyMmNvZGUlMjIlM0ElMjIlMkYlMkYlMjBjb21tZW50cyUyMGluJTIwYS5qcyU1Q25jb25zdCUyMGRhdGElM0QnZGF0YSclM0IlNUNuZXhwb3J0JTIwZGVmYXVsdCUyMGRhdGElM0IlNUNuJTIyJTJDJTIyaXNFbnRyeSUyMiUzQWZhbHNlJTJDJTIybmFtZSUyMiUzQSUyMmEuanMlMjIlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyb3V0cHV0JTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyZXMlMjIlN0QlMkMlMjJ0cmVlc2hha2UlMjIlM0FmYWxzZSU3RCU3RA==

Expected Behaviour

normal

all the comments are preserved , which only happens when the import statement is in the first line

Actual Behaviour

error

the import statement is hoisted to the beginning of the file ,and multiline comments are trimmed

from

// ==UserScript==
// @name        New Demo
// @namespace   Violentmonkey Scripts
// @match       <all_urls>
// @require     https://cdn.jsdelivr.net/npm/jquery@3
// @version     1.0
// ==/UserScript==

to

// ==UserScript==

similar issue: #3763

@lukastaegert
Copy link
Member

Fix at #4975

@rollup-bot
Copy link
Collaborator

This issue has been resolved via #4975 as part of rollup@3.21.5. You can test it via npm install rollup.

aarongable pushed a commit to chromium/chromium that referenced this issue Oct 2, 2023
Specifically updating:
 - svgo: 2.8.0  -> 3.0.2
   Includes a fix for [1] which was affecting some SVG files, and has
   resulted in using 'minifyStyles: false' in
   tools/resources/svgo.config.js as a workaround, see [2].
 - terser: 5.16.2 -> 5.20.0
 - rollup: 3.12.0 -> 3.29.4
 - eslint: 8.33.0 -> 8.50.0

With the latest version of Rollup, bundled files are unfortunately a
bit larger, due to fixing a previous bug [3], which incorrectly only
kept the first line of all license header comments. The new behavior
is correct, so perhaps we should look into ways of deduplicating license
header comments in the future, to further reduce the file size.

[1] svg/svgo#1672
[2] https://source.chromium.org/chromium/chromium/src/+/main:tools/resources/svgo.config.js;l=17-18;drc=4046a6ad0dfa25c3ec4ff3a0f2019badcd4ede70
[3] rollup/rollup#4953

Bug: None
Change-Id: I08fc9a6ad0cf37e75e0b5e3c4dcc61f4ad555838
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4903168
Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org>
Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1204162}
aarongable pushed a commit to chromium/chromium that referenced this issue Oct 2, 2023
…ns."

This reverts commit aca5179.

Reason for revert: 

May cause failures in https://ci.chromium.org/ui/p/chromium/builders/ci/linux-presubmit/8328/overview

Original change's description:
> WebUI: Update several toolchain dependencies to latest versions.
>
> Specifically updating:
>  - svgo: 2.8.0  -> 3.0.2
>    Includes a fix for [1] which was affecting some SVG files, and has
>    resulted in using 'minifyStyles: false' in
>    tools/resources/svgo.config.js as a workaround, see [2].
>  - terser: 5.16.2 -> 5.20.0
>  - rollup: 3.12.0 -> 3.29.4
>  - eslint: 8.33.0 -> 8.50.0
>
> With the latest version of Rollup, bundled files are unfortunately a
> bit larger, due to fixing a previous bug [3], which incorrectly only
> kept the first line of all license header comments. The new behavior
> is correct, so perhaps we should look into ways of deduplicating license
> header comments in the future, to further reduce the file size.
>
> [1] svg/svgo#1672
> [2] https://source.chromium.org/chromium/chromium/src/+/main:tools/resources/svgo.config.js;l=17-18;drc=4046a6ad0dfa25c3ec4ff3a0f2019badcd4ede70
> [3] rollup/rollup#4953
>
> Bug: None
> Change-Id: I08fc9a6ad0cf37e75e0b5e3c4dcc61f4ad555838
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4903168
> Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org>
> Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1204162}

Bug: None
Change-Id: I168e5d1a7f15b339dda21299698af046d30f5adf
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4908630
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org>
Auto-Submit: Jian Li <jianli@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1204278}
aarongable pushed a commit to chromium/chromium that referenced this issue Oct 3, 2023
…ons.

Reland notes: Fixed presubmit test failures in
 - tools/resources/svgo_presubmit_test.py
 - ui/webui/resources/tools/bundle_js_test.py

Specifically updating:
 - svgo:   2.8.0  -> 3.0.2
 - terser: 5.16.2 -> 5.20.0
 - rollup: 3.12.0 -> 3.29.4
 - eslint: 8.33.0 -> 8.50.0

rollup notes:
With the latest version of Rollup, bundled files are unfortunately a
bit larger, due to fixing a previous bug [1], which incorrectly only
kept the first line of all license header comments. The new behavior
is correct (see updated tests), so perhaps we should look into ways of
deduplicating license header comments in the future, to further reduce
the file size.

svgo notes:
 - Latest version includes a fix for [2] which was affecting some SVG
   files, and has resulted in using 'minifyStyles: false' in
   tools/resources/svgo.config.js as a workaround, see [3].
 - Updated svgo.config.js to account for the breaking change at [4].
 - Updated svgo_presubmit_test.py since it now re-arranges
   the order of SVG attributes as part of the optimization.

[1] rollup/rollup#4953
[2] svg/svgo#1672
[3] https://source.chromium.org/chromium/chromium/src/+/main:tools/resources/svgo.config.js;l=17-18;drc=4046a6ad0dfa25c3ec4ff3a0f2019badcd4ede70
[4] svg/svgo@6295c60

Bug: None
Change-Id: I12ca6f1204868a8592d03b24852c12c97e4e5b0b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4908394
Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1204712}
@lukastaegert
Copy link
Member

The assumption is that the comments at the start of main.js describe the contents of main.js and the comments at the start of a.js describe the contents a.js. Hence, they are in the correct position, above the content of each file.

@viocha
Copy link
Author

viocha commented Oct 21, 2023

The assumption is that the comments at the start of main.js describe the contents of main.js and the comments at the start of a.js describe the contents a.js. Hence, they are in the correct position, above the content of each file.

Thank you for taking the time to respond.
I know the import statement should be written at the beginning of the js file. But sometimes, I need to write some comments before the import statements. If I run rollup, the import statement will be hoisted before the comments, causing the code of a.js module to appear before the comments in main.js. It would be great if rollup could provide an option to prevent the hoisting of import statements.
In addition, even if treeshaking is disabled, some comment information in the main.js file is still not retained, as shown in the picture.

@lukastaegert
Copy link
Member

Imports are not hoisted, they are completely removed and recreated based on what cannot be found in a chunk. That does not make a lot of difference to users, but explains why we cannot just "not hoist" imports. If several modules in a chunk import the same binding from a dependency, we need to merge those exports to produce valid and efficient code. The comment logic is like this: A comment on a separate line is considered to describe the next statement below. If that statement is removed, then the comment needs to be removed with it. The only exception are comments at the beginning of a file not separated by empty lines as those are considered to describe the file instead and are retained if any content of that file is not removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants