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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve pure comments #550

Merged
merged 4 commits into from
Jan 10, 2020
Merged

Preserve pure comments #550

merged 4 commits into from
Jan 10, 2020

Conversation

edoardocavazza
Copy link
Contributor

Right now @__PURE__ comments are drop from the final file, also if the comment option is all. In addition to being an inconsistent behavior, this strategy can be dangerous if terser is applied to an ES module, since those files can be consumed by browsers AND bundlers like Rollup which handle the pure annotation.

This PR also prevent terser from adding empty comments as described by #526.

Please consider this PR as a simple suggestion, I know that it could be a potential breaking change and that it can implement an unwanted behavior 馃槄

@fabiosantoscode
Copy link
Collaborator

Hey there!

What is your specific use case? I think that if rollup had a problem with the removal of pure comments @TrySound or someone else working on rollup would've brought it up at this point. Either to Terser or UglifyJS, which does the same thing. So please let me know, what is the use case?

@TrySound
Copy link
Contributor

TrySound commented Jan 8, 2020

I think the point is distributing preminified esm which could be consumed by bundlers. Though probably not only lack of pure annotation may prevent rollup optimisations. IMO better to run terser or any other minifier in the end of app build process.

@edoardocavazza
Copy link
Contributor Author

Hello! IMO, in the near future the usage of import/export in the browsers will grow a lot and distributing a single module entry point for bundlers and browsers (for example, using unpkg.com) simplifies a lot the workflow for maintainers and developers. As @TrySound pointed out, Rollup can not optimize final bundles if the pure annotation is missing. Furthermore, at the moment we are not able to handle @__PURE__ as any other comment because they are always stripped out. Do you think that introducing a new option for this case can make sense?

@edoardocavazza
Copy link
Contributor Author

Please note that this PR does not make terser to ALWAYS preserve pure annotation, but to treat them as any other comment. You can still remove any comment or filter them out via option.

@fabiosantoscode
Copy link
Collaborator

You are both making a lot of sense here, thanks for your input :) but let's add a preserve_annotations output option (which defaults to false).

This is going to make the project harder to maintain, but it seems like there's a real need for this.

And thanks for removing those awkward empty comments :)

If you don't mind, please update the typescript typings to reflect this new option. If you don't I can always do that later so don't worry.

@edoardocavazza
Copy link
Contributor Author

edoardocavazza commented Jan 9, 2020

Hi @fabiosantoscode , I added the option, can you please review my commits?

Copy link
Collaborator

@fabiosantoscode fabiosantoscode left a comment

Choose a reason for hiding this comment

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

Hey there! Everything looks great, just that one change I pointed out in the review. Thanks for your contribution!

bin/terser Outdated Show resolved Hide resolved
@fabiosantoscode
Copy link
Collaborator

Super cool! I'll release a version with this today or tomorrow. Thank you very much!

@danieltroger
Copy link

Hi, how can I enable this feature? cat module.js|terser -cm --comments "/__PURE__/" does not produce the desired result (with pure comments preserved)

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.

None yet

4 participants