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.

Sign up for GitHub

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

Jump to bottom

feat: add support for preserving comments in code #967

Closed
wants to merge 2 commits into from
Closed

feat: add support for preserving comments in code #967

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
227989b
feat: add support for preserving comments in code
tido64 Apr 17, 2023
bd567e5
let the minifier handle comments
tido64 Apr 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/metro-config/src/__tests__/__snapshots__/loadConfig-test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ Object {
},
"output": Object {
"ascii_only": true,
"comments": false,
"quote_style": 3,
"wrap_iife": true,
},
Expand Down Expand Up @@ -310,6 +311,7 @@ Object {
},
"output": Object {
"ascii_only": true,
"comments": false,
"quote_style": 3,
"wrap_iife": true,
},
Expand Down Expand Up @@ -488,6 +490,7 @@ Object {
},
"output": Object {
"ascii_only": true,
"comments": false,
"quote_style": 3,
"wrap_iife": true,
},
Expand Down Expand Up @@ -666,6 +669,7 @@ Object {
},
"output": Object {
"ascii_only": true,
"comments": false,
"quote_style": 3,
"wrap_iife": true,
},
Expand Down
1 change: 1 addition & 0 deletions packages/metro-config/src/defaults/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ const getDefaultValues = (projectRoot: ?string): ConfigT => ({
},
output: {
ascii_only: true,
comments: false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I don't see a need to specify this explicitly.
  2. Our default minifier, Terser, has a documented default of "some", set here, so this overrides that and may be an unwanted change in behaviour. Per 1, let's just remove this explicit Metro default.
Suggested change
comments: false,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, we had comments: false in Babel, which basically omits all comments, so no comments ever reach the minifier. To preserve that behaviour, we need to set comments: false here. If you still want to remove it, that's fine with me, but it will produce a different output than today.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see! Yeah, in that case we should probably land this with a behaviour-preserving default so it can go out as a patch, then align with terser's default in the next major. (Would you mind sending a separate PR with that?)

The only thing I don't get is: if this is supposed to be behaviour-preserving, why did the buildGraph test snapshot change? Do we have a good test covering the actual default from Metro's config?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It changed because it's unminified, so the comments have yet to be stripped out.

I can send out a separate PR to remove this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for merging this PR! Here's the follow-up: #972

quote_style: 3,
wrap_iife: true,
},
Expand Down
Loading