Skip to content

ref(build): Remove all comments in generated output #5799

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

lforst
Copy link
Contributor

@lforst lforst commented Sep 22, 2022

This PR is based on #5783 and the contained discussion.

Quoted from that PR:

The existing regex, when presented with code like this:

/* 
 * This is
 * a multiline
 * comment 
 */

const thisIs = "real code"

/* 
 * This is
 * another multiline
 * comment 
 */

const thisIsAlso = "real code"

matches everything between the initial /* on the first line and the final */ on the second-to-last line, and the plugin thus removes all but the last line of the file. This fixes the regex so that only the comments are removed.


In order to avoid similar bugs, and since comment-removal is not as simple as applying a regex, we should outsource comment deletion to a dedicated library - in this case rollup-plugin-cleanup. I decided to remove all comments instead of only multi-line ones because the upside of only having certain ones is unclear to me.

Important comments like tree-shaking hints or source map comments are still intact.

@lforst lforst changed the title ref(build): Remove all comments in generated files ref(build): Remove all comments in generated output Sep 22, 2022
"rollup-plugin-license": "^2.6.1",
"rollup-plugin-re": "^1.0.7",
"rollup-plugin-terser": "^7.0.2",
Copy link
Member

Choose a reason for hiding this comment

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

we should change all of these to be hard pins - we can do it in another PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do!

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

I'm in favour of this change, since I think we should be indiscriminately removing all comments - it's easier to understand and debug + we don't have to worry about maintaining regex.

@github-actions
Copy link
Contributor

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.43 KB (-0.22% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 60.08 KB (-0.14% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.06 KB (-0.01% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 53.01 KB (-0.13% 🔽)
@sentry/browser - Webpack (gzipped + minified) 19.82 KB (-0.2% 🔽)
@sentry/browser - Webpack (minified) 64.37 KB (-0.18% 🔽)
@sentry/react - Webpack (gzipped + minified) 19.84 KB (-0.2% 🔽)
@sentry/nextjs Client - Webpack (gzipped + minified) 44.74 KB (+0.07% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 25.88 KB (-0.13% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.31 KB (+0.02% 🔺)

@lobsterkatie
Copy link
Member

lobsterkatie commented Sep 27, 2022

Discussed offline and we're going to go ahead with a modified version of this, which doesn't remove comments.

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.

3 participants