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

Already on GitHub? Sign in to your account

minify helpers-generated.ts #13837

Merged
merged 5 commits into from Nov 18, 2021

Conversation

lightmare
Copy link
Contributor

@lightmare lightmare commented Oct 11, 2021

Q                       A
Fixed Issues? none
Patch: Bug Fix? no
Major: Breaking Change? hopefully not
Minor: New Feature?
Tests Added + Pass? none added
Documentation PR Link
Any Dependency Changes? babel-helpers devdeps: + terser; additionally yarn dedupe upgraded terser and source-map-support
License MIT

I came across this comment in babel-helpers/scripts/generate-helpers.js:

// TODO: We can minify the helpers in production

and thought it should not be difficult to at least strip unnecessary whitespace, of which there is a lot in the output.

Given that the helpers don't (can't?) contain template strings, which would require properly parsing the source to avoid removing important whitespace, redundant whitespace outside string literals can quite easily be removed with a few regex replacements.

Another trick is to export a single object from helpers-generated.ts, instead of individual constants, which avoids the need to rename identifier-unfriendly helper like typeof to _typeof and back.

edit: Changed to properly minify helpers with terser. 🛩️

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 11, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9be85ca:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@babel-bot
Copy link
Collaborator

babel-bot commented Oct 11, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/49856/

@JLHwung
Copy link
Contributor

JLHwung commented Oct 12, 2021

I am conservative on a mini-minifier used in babel-helpers only for maintenance cost. Can we run terser on helper sources?

@lightmare
Copy link
Contributor Author

I am conservative on a mini-minifier used in babel-helpers only for maintenance cost.

Agreed. I wasn't too confident about this to begin with, and have since encountered two potential issues -- one is that this cannot recognize a regex literal in the source, so could potentially destroy it; the other is with eating semicolons. I had to put two in a helper with an empty {while(stuff());;} loop, because one was being removed, and the most frustrating part about it was convincing both eslint and prettier that the two ;; must be there :D

Can we run terser on helper sources?

Looking into it now, and it seems pretty straightforward. With mangle: false, compress: false it produces slightly smaller output that this PR (but more importantly, always correct); and with compress: true, mangle: false it's even smaller, yet still kinda readable as identifiers are kept.

@lightmare lightmare marked this pull request as draft October 12, 2021 17:15
@lightmare
Copy link
Contributor Author

I'm having trouble committing changed generate-helpers.js using terser.

✖ eslint --format=codeframe found some errors. Please fix them and try committing again.
error: 'dist' should be listed in the project's dependencies. Run 'npm i -S dist' to add it (import/no-extraneous-dependencies) at packages/babel-helpers/scripts/generate-helpers.js:4:1:
2 | import { join } from "path";
3 | import { URL, fileURLToPath } from "url";
> 4 | import { minify } from "terser";
  | ^

Said package dist is inside the installed terser package, but it's not a real dependency that's supposed to be tracked independently, so that npm -i -S dist suggestion doesn't help much.

Should I just eslint-disable the one import line, or is there a proper fix for this kind of issue?

@nicolo-ribaudo
Copy link
Member

Whops sorry, I didn't notice your question. eslint-disable-next-line is fine.

@lightmare lightmare changed the title fake-minify helpers-generated.ts minify helpers-generated.ts Nov 5, 2021
@lightmare
Copy link
Contributor Author

Rebased on main, and replaced the hacky fake-minify-by-regexp with calling terser.

@lightmare lightmare marked this pull request as ready for review November 5, 2021 22:17
// Remove multiple newlines
.replace(/\n{2,}/g, "\n");
const source = await minify(fileContents, {
compress: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the default compressor option break babel helper? May leave a note/todo here.

I think we disable mangle because babel helper does not ship with a source map.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

compress: true works too, I disabled it only to keep the code as is (compress rewrites some expressions, moves declarations and conditionals into for loop initialization, etc.); if readability is not a concern here, it can be enabled.

Copy link
Member

Choose a reason for hiding this comment

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

As long as debuggability is not affected too much (I want to be able to understand stack traces in bug reports), that's ok.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah the stack traces will be fine since we have disabled mangle, which renames identifiers to the shortest available.

@JLHwung JLHwung added area: helpers PR: Output optimization 🔬 A type of pull request used for our changelog categories labels Nov 18, 2021
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Thanks! I went ahead and enabled compression so that we can merge this. I had to tweak the custom plugin we use to handle module.exports because it was causing infinite loops when running on the compressed code.

lightmare and others added 5 commits November 18, 2021 23:28
This obviates the need to carefully handle helper names that cannot be
directly used as identifiers, and also very slightly reduces the amount
of boilerplate that ends up in babel-standalone.
@nicolo-ribaudo nicolo-ribaudo merged commit cd77ad1 into babel:main Nov 18, 2021
@lightmare
Copy link
Contributor Author

... it was causing infinite loops when running on the compressed code.

Interesting, haven't noticed that. It's quite likely I disabled compress right after seeing the output, without first running all tests with it. I thought it was purely cosmetic.

@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Feb 20, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 20, 2022
@lightmare lightmare deleted the shrink-helpers-generated branch September 2, 2023 19:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: helpers outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Output optimization 🔬 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants