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

Improve output of super() #16194

Merged
merged 8 commits into from Jan 8, 2024
Merged

Conversation

liuxingbaoyu
Copy link
Member

Q                       A
Fixed Issues? Fixes #1, Fixes #2
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

This is a small change, and if we can merge it before the next release, we won't need to add a new-version-checklist. :) (Some of my other PRs are a bit troublesome to rebase)

@babel-bot
Copy link
Collaborator

babel-bot commented Dec 26, 2023

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

@liuxingbaoyu liuxingbaoyu added Spec: Classes PR: Output optimization 🔬 A type of pull request used for our changelog categories labels Dec 26, 2023
@@ -274,6 +274,7 @@ module.exports = [
"no-func-assign": "off",
"import/no-extraneous-dependencies": "off",
"import/no-unresolved": "off",
"@typescript-eslint/prefer-optional-chain": "off",
Copy link
Contributor

Choose a reason for hiding this comment

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

@JoshuaKGoldberg If we set parserOptions.ecmaVersion to 5, will typescript-eslint respect the ecmaVersion and automatically turn off this rule?

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting idea! No, rules in the preset configs won't be configured differently based on your parserOptions. I don't believe ESLint configs can be dynamic in this way. At least legacy ones aren't, and we haven't added official support for flat configs yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

@JoshuaKGoldberg Thank you! I think this is not a very common use case, since most TS projects will use tsc to transpile ts sources anyway.

@liuxingbaoyu Can we set parserOptions.ecmaVersion to 5 for helpers? So the linter should throw if there are non-ES5 codes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Parsing error: ImportDeclaration should appear when the mode is ES6 and in the module context.
I tried it and it didn't work.
Our current ts source actually has some es6 syntax such as trailingComma handled by terser which I think is fine.
I have now unified js with ts and explicitly specified terser as es5.

@liuxingbaoyu
Copy link
Member Author

Thanks for the tag! Now that we have released v7.23.7, I removed it as it should be included in the next release no later than later. :)

@liuxingbaoyu liuxingbaoyu marked this pull request as draft December 30, 2023 04:13
@liuxingbaoyu
Copy link
Member Author

Luckily github fixed their CI issue, this PR has test262 failure, I will fix it.

@liuxingbaoyu liuxingbaoyu marked this pull request as ready for review December 30, 2023 04:28
@nicolo-ribaudo
Copy link
Member

When compiling this code:

class A extends B {
  constructor(arg1, arg2, arg3) {
    super(arg1, arg2);
    this.arg3 = arg3;
  }
}

targeting IE11, and manually replacing helper names with one-char variables when minifing, the output size is:

Before this PR After this PR
Size 315 bytes 123 bytes
Minified size 287 bytes 111 bytes

The old code is slightly faster in some very old browsers when compiling implicit constructors, because we pass arguments to .apply that had optimized handling. However, now the "pass arguments to another function" deopt doesn't exist anymore.

@liuxingbaoyu
Copy link
Member Author

Now we avoid a function call/new closure, hoping that this can offset performance changes. : :)

@nicolo-ribaudo
Copy link
Member

I'll avoid the new-version-checklist change, hoping that the next release will be a patch :)

@nicolo-ribaudo nicolo-ribaudo merged commit 7840c54 into babel:main Jan 8, 2024
48 checks passed
@liuxingbaoyu
Copy link
Member Author

Yeah, due to the regression fix #16199, we are enough to release a patch version. : :)

@steverep
Copy link

Hi, could this change have caused a regression in output size? Home Assistant is seeing a hefty 4% bundle size increase from 7.23.7 to 7.23.8 (home-assistant/frontend#19368).

Looking at the release notes, this PR seems like the only viable culprit.

@steverep
Copy link

Took a deeper look and it seems the bundle is no longer using @babel/runtime/helpers/esm/createSuper.js, so a significant portion of our transpiled modules went up by those ~800 bytes.

@liuxingbaoyu
Copy link
Member Author

The purpose of this PR is to reduce the output size, but it may cause the size to increase when users use the new @babel/plugin-transform-classes and the old @babel/helper.

I noticed home-assistant/frontend@349c6cf (#19368), does this solve the problem?

@steverep
Copy link

Yes, I noticed @babel/helpers was still locked at 7.23.7 and bumped it. That did seem to fix the problem.

@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 Apr 13, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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 Spec: Classes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants