Skip to content

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Aug 24, 2023

This is useful for folks who want to run the closure compiler as an external step, after linking.

Fixes google-internal issue: https://buganizer.corp.google.com/issues/287520718

@sbc100 sbc100 requested a review from kripken August 24, 2023 18:20
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Makes sense to me, because --minify=0 is only used for debugging and not production builds. For a debug builds, adding comments is no burden and can only help.

Maybe worth mentioning that in the changelog and the setting's docs?

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 24, 2023

I did update the changelog.

ChangeLog.md Outdated
@@ -21,6 +21,9 @@ See docs/process.md for more on how version tagging works.
3.1.46 (in development)
-----------------------
- libunwind updated to LLVM 16.0.6. (#20088)
- The `--minify=0` commnad line flag will now preserve comments as well as
whitespace. This means the resulting output can then be run though closure
compiler or some other tool that gives comments semantic meaning.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
compiler or some other tool that gives comments semantic meaning.
compiler or some other tool that gives comments semantic meaning (and
should have no downside, as `--minify=0` is only used in debug builds anyhow).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure about that though... --minify=0 isn't just for debug builds.. it might be useful in release builds too, if folks want to do their own minification later on the pipeline.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, maybe you're right. I just think it would be good to explain why we don't think this will inconvenience existing users. So it's either a debug build, or one that the user will minify later anyhow, and either way we aren't hurting anything.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah .. I guess we could say something like.. "which is in keeping the with spirit of minify=0 which is designed for folks who either want to debug their code to perform their own minification". I'm not sure its worth the extra sentence though.

@kripken
Copy link
Member

kripken commented Aug 24, 2023

Sounds good.

I added a comment with an idea for a further explanation in the changelog.

This is useful for folks who want to run the closure compiler as an
external step, after linking.
@sbc100 sbc100 force-pushed the preserve_comments branch from eaca395 to 70418c7 Compare August 24, 2023 20:21
@sbc100 sbc100 merged commit a788f15 into main Aug 24, 2023
@sbc100 sbc100 deleted the preserve_comments branch August 24, 2023 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants