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

Properly handle class properties proposal #420

Merged
merged 1 commit into from
Dec 14, 2021

Conversation

chriskrycho
Copy link
Contributor

Historically, we only add the @babel/plugin-proposal-class-properties so that we make sure the ordering is right with the decorators proposal (otherwise, it can end up compiling in the wrong order). With a recent version of @babel/preset-env and, transitively, caniuse-lite, this resulted in cases where we added that plugin but not related plugins for private class properties, which in turn triggered a Babel assertion about not adding the properties together as appropriate when the caniuse database (correctly) reported that .

The fix is:

  1. Bump to a more recent version of @babel/preset-env, which comes with a correspondingly bumped version of caniuse-lite, which in turn correctly understands what the latest versions of targeted browsers are.

  2. Include in ember-cli-babel itself a check for whether we even need to add the plugin, and only provide it when the provided targets indicate that they require it.

Resolves #419

chriskrycho added a commit to chriskrycho/do-it-break that referenced this pull request Dec 7, 2021
@chriskrycho
Copy link
Contributor Author

chriskrycho commented Dec 7, 2021

Tested this, besides the unit tests, by doing a yarn pack and directly referenced the resulting .tgz and confirmed it works in do-it-break. You should also be able to use the PR by GitHub reference and confirm it works, and you can also check out the latest version of do-it-break and see that it is now fixed there by referencing this commit.

@@ -48,7 +48,7 @@
"@babel/plugin-transform-runtime": "^7.13.9",
"@babel/plugin-transform-typescript": "^7.13.0",
"@babel/polyfill": "^7.11.5",
"@babel/preset-env": "^7.12.0",
"@babel/preset-env": "^7.16.4",
"@babel/runtime": "7.12.18",

Choose a reason for hiding this comment

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

Might be worth it to try and unpin @babel/runtime again after this PR too (Discussion in #384 )

@NullVoxPopuli
Copy link
Sponsor Contributor

NullVoxPopuli commented Dec 8, 2021

I confirm that with this branch, things work as expected!

image
image

I expect some perf / build improvements from this! let's goooooo 🚢

nice work @chriskrycho <3

@rwjblue
Copy link
Member

rwjblue commented Dec 8, 2021

Rebased / amended to kick off a CI run

Historically, we only add the `@babel/plugin-proposal-class-properties`
so that we make sure the ordering is right with the decorators proposal
(otherwise, it can end up compiling in the wrong order). With a recent
version of `@babel/preset-env` and, transitively, `caniuse-lite`, this
resulted in cases where we added that plugin but *not* related plugins
for private class properties, which in turn triggered a Babel assertion
about not adding the properties together as appropriate when the caniuse
database (correctly) reported that .

The fix is:

1.  Bump to a more recent version of `@babel/preset-env`, which comes
    with a correspondingly bumped version of `caniuse-lite`, which in
    turn correctly understands what the latest versions of targeted
    browsers are.

2.  Include in `ember-cli-babel` itself a check for whether we even
    *need* to add the plugin, and only provide it when the provided
    `targets` indicate that they require it.

Resolves emberjs#419
@rwjblue rwjblue added the bug label Dec 8, 2021
@chriskrycho
Copy link
Contributor Author

nice work @chriskrycho <3

Totally a team effort with @rwjblue – we spent a half day pairing on it till we had a solution; I just did the "write some tests and open a PR" bit. ;)

@NullVoxPopuli
Copy link
Sponsor Contributor

Thanks, @rwjblue !!!!! <3

I super excited to deploy these updates everywhere!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Class private methods are not enabled.
4 participants