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

Fix decorators babel plugin issue #426

Merged
merged 5 commits into from
Dec 16, 2021
Merged

Conversation

chriskrycho
Copy link
Contributor

@chriskrycho chriskrycho commented Dec 16, 2021

Starts by introducing a failing test case for #423, #425. Will also introduce more, and hopefully will fix this. Wheeeeeee.


Current working theory, based on reading the code for the Babel decorators plugin: we must actively, manually supply all the class properties (including private class properties and methods) plugins to make sure it works, since we're manually opting ourselves into decorators support.

If that's true, our fix will look like:

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Dec 16, 2021

I'm curious why this "Works" then: https://github.com/NullVoxPopuli/babel-transpilation-tests

export default {
  presets: ['@babel/preset-env'],
  plugins: [['@babel/plugin-proposal-decorators', { legacy: true }]]
};

babel hurts my head lol

browsers: ["last 2 chrome versions"],
};

subject = this.addon.transpileTree(input.path(), {});
Copy link
Contributor

Choose a reason for hiding this comment

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

oh! this confirms my hypothesis from: #422 (comment)

yay!

@rwjblue
Copy link
Member

rwjblue commented Dec 16, 2021

I'm curious why this "Works" then: NullVoxPopuli/babel-transpilation-tests

Mentioned in discord, but I'll also mention it here: that repro actually demonstrates the failure!!!

See this assertion: https://github.com/NullVoxPopuli/babel-transpilation-tests/blob/9bbc2c8353e45bebae9ae16defd40830614718e1/output.js#L20

@NullVoxPopuli
Copy link
Contributor

Mentioned in discord, but I'll also mention it here: that repro actually demonstrates the failure!!!

I'm sad to see this! lol.
I hadn't ran that code, because I figured it was small enough to "just be right" -- focusing primarily on the non-transpilation of the class properties, and the "supposed" transpilation of the decorators.
Looking at now (it's been a while since I made that repo), it's very obvious that the problem is the same as what we saw with ember-cli-babel 7.26.7 :(

@chriskrycho chriskrycho force-pushed the decorators-fun branch 2 times, most recently from f138137 to 64e00a6 Compare December 16, 2021 22:37
@chriskrycho chriskrycho changed the title [WIP] fix decorators babel plugin issue Fix decorators babel plugin issue Dec 16, 2021
@chriskrycho chriskrycho marked this pull request as ready for review December 16, 2021 22:38
chriskrycho and others added 4 commits December 16, 2021 18:40
This is not particularly optimal: it means consumers end up doing this
tranpilation even when they only support targets which already *do*
support class fields. However, we currently have to do this to support
the ecosystem unless or until we fix (or fork) the Babel decorators
legacy transform since it [injects a function which errors at runtime
if it ever sees a class field][1]

[1]: https://github.com/babel/babel/blob/ad17fe1cceadd4df4bd43d3d13619bb349fc7d8a/packages/babel-plugin-proposal-decorators/src/transformer-legacy.ts#L155
This is bad, I feel bad.

There is no way to enable parser support for private methods in this
version of eslint, and we can't update eslint versions unless we drop
node version support (requiring a major release).
@rwjblue rwjblue added the bug label Dec 16, 2021
@rwjblue rwjblue merged commit f8f495e into emberjs:master Dec 16, 2021
@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Dec 16, 2021

Just tested this out, and it's an improvement!
image
I no longer see defineProperty, so this is a huge improvement! thank you both <3

(previously):
image

@chriskrycho chriskrycho deleted the decorators-fun branch December 17, 2021 00:24
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.

3 participants