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

[Feature]: provide better comment annotation #7591

Closed
hardfist opened this issue Aug 16, 2024 · 14 comments · Fixed by webpack/webpack#18713
Closed

[Feature]: provide better comment annotation #7591

hardfist opened this issue Aug 16, 2024 · 14 comments · Fixed by webpack/webpack#18713
Labels
feat New feature or request stale team The issue/pr is created by the member of Rspack.

Comments

@hardfist
Copy link
Contributor

hardfist commented Aug 16, 2024

What problem does this feature solve?

currently rspack | webpack will generate the following result for modern-module, the comment is not nicely(Concate module seems hard to understand for normal users) to read, even minifier can remove these comments, but normally library code shouldn't be minified for better debug experience

;// CONCATENATED MODULE: ./src/answer.js
const answer = 42;

;// CONCATENATED MODULE: ./src/secret.js
const secret = 'rspack';

;// CONCATENATED MODULE: ./src/index.js



export { answer, secret };

//# sourceMappingURL=index.mjs.map

What does the proposed API of configuration look like?

there're some solutions, i'm not sure which is better

  • provide a option to remove comments (like rollup don't have this annotation)
{
  output: {
      keepDebugComments: boolean // true means keep, false means remove
   }
}
  • provide better comment annotation(like esbuild provide the simplest annotation)
 ./src/answer.js
const answer = 42;

 ./src/secret.js
const secret = 'rspack';

 ./src/index.js
@hardfist hardfist added feat New feature or request pending triage The issue/PR is currently untouched. labels Aug 16, 2024
@github-actions github-actions bot added the team The issue/pr is created by the member of Rspack. label Aug 16, 2024
@hardfist
Copy link
Contributor Author

@alexander-akait is this option acceptable for webpack or any better suggestions for this problem? also cc @fi3ework

@hardfist hardfist removed the pending triage The issue/PR is currently untouched. label Aug 16, 2024
@chenjiahan
Copy link
Member

The original module path (./src/answer.js) is useful for debugging, maybe we can replace CONCATENATED MODULE with another words, or only remove CONCATENATED MODULE.

@chenjiahan
Copy link
Member

We can consider adding an rspackFuture.friendlyComments option to generate more user friendly comments, and it can be enabled in Rspack v2 by default.

@hardfist
Copy link
Contributor Author

We can consider adding an rspackFuture.friendlyComments option to generate more user friendly comments, and it can be enabled in Rspack v2 by default.

comments content shouldn't be public api, so change comment content seems not a breaking change

@chenjiahan
Copy link
Member

Yes, if Rspack doesn't strive for consistency with webpack's outputs and comments, then changing the comments content directly is a better idea.

@hardfist hardfist changed the title [Feature]: support remove comments in development mode [Feature]: provide better comment annotation Aug 16, 2024
@alexander-akait
Copy link

I am fine to improve comment, maybe we can do it without any new options for output, it is just a debug infromation, so we can just impove it

@alexander-akait
Copy link

How do you want to see them?

@hardfist
Copy link
Contributor Author

How do you want to see them?

to me, the esbuild's comment is the simplest and clean

➜  webpack-demo esbuild src/index.mjs --bundle      
"use strict";
(() => {
  // src/answer.mjs
  var answer = 42;

  // src/secret.mjs
  var secret = "rspack";

  // src/index.mjs
  console.log({
    answer,
    secret
  });
})();

the module path is already contains enough info for debugging and seems no need to keep the CONCATENATED MODULE:(which is hard for users who don't know webpack's internal)

@alexander-akait
Copy link

So you just want to remove CONCATENATED MODULE words?

@fi3ework
Copy link
Member

So you just want to remove CONCATENATED MODULE words?

Yes, that will sufficient.

And it will be nicer to adding the leading ; introduced in webpack/webpack#11920 only when necessary, if possible.

@alexander-akait
Copy link

@fi3ework @hardfist Feel free to send a Pull requests

And it will be nicer to adding the leading ; introduced in webpack/webpack#11920 only when necessary, if possible.

I think we already insert it only when needed, if not, consider it as a bug fix

Copy link

stale bot commented Oct 19, 2024

This issue has been automatically marked as stale because it has not had recent activity. If this issue is still affecting you, please leave any comment (for example, "bump"). We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@stale stale bot added the stale label Oct 19, 2024
@fi3ework
Copy link
Member

bump, already landed in webpack.

@stale stale bot removed the stale label Oct 19, 2024
Copy link

stale bot commented Dec 18, 2024

This issue has been automatically marked as stale because it has not had recent activity. If this issue is still affecting you, please leave any comment (for example, "bump"). We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@stale stale bot added the stale label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request stale team The issue/pr is created by the member of Rspack.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants