feat: add support for preserving comments in code #967
Conversation
facebook-github-bot
added
the
CLA Signed
|
Thanks! This seems pretty well defined - I wonder whether we could just go straight to stable with this. WDYT @motiz88? |
@robhogan: Makes sense to me. I guess this is something all transformers (even non-Babel-based ones) would be able to implement, so no concerns there. The only real gap IMO is that we should document this in https://facebook.github.io/metro/docs/configuration/#transformer-options if stable. To avoid confusion, I wouldn't explicitly mention tree-shaking, and instead just briefly describe what the option itself does. |
tido64
force-pushed
the
tido/preserve-comments
branch
2 times, most recently
from
967c64a to
6ba1071
Compare
Useful for preserving `/*#__PURE__*/` annotations that aid tree shaking.
tido64
force-pushed
the
tido/preserve-comments
branch
from
6ba1071 to
bd567e5
Compare
| @@ -111,6 +111,7 @@ const getDefaultValues = (projectRoot: ?string): ConfigT => ({ | |||
| }, | |||
| output: { | |||
| ascii_only: true, | |||
| comments: false, | |||
There was a problem hiding this comment.
- I don't see a need to specify this explicitly.
- 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.
| comments: false, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks for merging this PR! Here's the follow-up: #972
|
@motiz88 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary
Add support for preserving comments in code. Useful for preserving
/*#__PURE__*/annotations that aid tree shaking.Test plan
Test case here: microsoft/rnx-kit#2365