feat: add comments in module code generation

[faga295]

Related issue (if exists)

#2868

Summary

🤖 Generated by Copilot at 15c7690

Add support for comments in the JavaScript plugin. Modify the stringify function in stringify.rs to pass the comments from the AST to the print function.

Walkthrough

🤖 Generated by Copilot at 15c7690

• Preserve comments in AST when stringifying JavaScript code (link)

[changeset-bot]

🦋 Changeset detected

Latest commit: 0587b99

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 17 packages

Name	Type
@rspack/binding	Patch
@rspack/postcss-loader	Patch
@rspack/core	Patch
webpack-test	Patch
@rspack/cli	Patch
@rspack/dev-middleware	Patch
@rspack/dev-server	Patch
@rspack/plugin-html	Patch
benchmarkcase-rspack-react-refresh	Patch
@rspack/dev-client	Patch
@rspack/plugin-minify	Patch
@rspack/plugin-node-polyfill	Patch
@rspack/binding-darwin-arm64	Patch
@rspack/binding-darwin-x64	Patch
@rspack/binding-linux-x64-gnu	Patch
@rspack/binding-win32-x64-msvc	Patch
@rspack/fs	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[faga295]

this pr also includes some expected file change, and the tree-shaking/ts-target-es5's test seems has some diff problem, it's failed even though the expected file is same as actual file, and I'm very confused about this.

[IWANABETHATGUY]

Would you mind adding an option to control this behavior? Not everyone wants comments in their outputs, also this would slow the codegen phase. You could add some tests in another dir, comments are kinds of noise in tree shaking test.

[faga295]

Ok, I will add comments option in minifyOption, and the default value is false.

[IWANABETHATGUY]

I don't think it is a good option to mix this behavior with minifyOption, because according to your implementation, this option should control development mode code generation behavior as well. Would you mind waiting about one day or so? Maybe we could discuss where should we put the option, or we don't need a option just codegenerate the comments anyway.

[faga295]

Hey @IWANABETHATGUY , after discussing how this option should be designed🤔️

[IWANABETHATGUY]

We don't want this feature to introduce too much overhead, so let's bench it first.
!bench

[github-actions]

Benchmark Results

group                                                 baseline                                 pr
-----                                                 --------                                 --
criterion_benchmark/ten_copy_of_threejs               444.86      2.5±0.08s        ? ?/sec     1.00      5.5±0.97ms        ? ?/sec
high_cost_benchmark/ten_copy_of_threejs_production    1178.94      8.8±0.13s        ? ?/sec    1.00      7.4±0.66ms        ? ?/sec

[IWANABETHATGUY]

!bench

[IWANABETHATGUY]

please make sure to pass the CI, you could run UPDATE=1 cargo test -p rspack

[github-actions]

Benchmark Results

group                                                 baseline                               pr
-----                                                 --------                               --
criterion_benchmark/ten_copy_of_threejs               1.00       3.1±0.14s        ? ?/sec    1.07       3.3±0.30s        ? ?/sec
high_cost_benchmark/ten_copy_of_threejs_production    1.00      11.0±0.23s        ? ?/sec    1.01      11.1±0.38s        ? ?/sec

[IWANABETHATGUY]

!bench

[github-actions]

Benchmark Results

group                                                 baseline                               pr
-----                                                 --------                               --
criterion_benchmark/ten_copy_of_threejs               1.00       2.3±0.08s        ? ?/sec    1.03       2.4±0.10s        ? ?/sec
high_cost_benchmark/ten_copy_of_threejs_production    1.00       8.1±0.10s        ? ?/sec    1.01       8.2±0.25s        ? ?/sec

[faga295]

this pr also includes some expected file change, and the tree-shaking/ts-target-es5's test seems has some diff problem, it's failed even though the expected file is same as actual file, and I'm very confused about this.

the tree-shaking/ts-target-es5 is strange, after run UPDATE=1 cargo test -p rspack, test can pass locally, but no files changed

[IWANABETHATGUY]

Do you use Windows?

[faga295]

Do you use Windows

OS: macOS 13.0
CPU: (8) arm64 Apple M1 Pro

There is really no difference between the expected file and the actual file

rspack/crates/cargo-rst/src/rst.rs

Lines 347 to 363 in 4e25faf

	if is_expect_file && is_actual_file {
	let expected_buf = fs::read(expected_dir.as_path()).expect("TODO:");
	let expected_str = String::from_utf8_lossy(&expected_buf);
	let actual_buf = fs::read(actual_dir.as_path()).expect("TODO:");
	let actual_str = String::from_utf8_lossy(&actual_buf);
	if expected_str != actual_str {
	let diff = FileDiff {
	expected_path: expected_dir.clone(),
	actual_path: actual_dir.clone(),
	expected_content: expected_str.to_string(),
	actual_content: actual_str.to_string(),
	};
	err.push(TestErrorKind::Difference(diff))
	}

Is this problem due to diff?

[IWANABETHATGUY]

I don't think so,


It has some diff but will be replaced by git when you commit to GitHub.

[IWANABETHATGUY]

I think you could add an option to control this behavior, like builtins.codeGeneration.keepComments

[IWANABETHATGUY]

By default this option should be disabled, you could add extra test case to test this behavior, this will not conflict with current test case.

[IWANABETHATGUY]

Thanks!