Skip to content

[BUGFIX] Removes the class properties transform for the latest ember-cli-babel #640

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

Conversation

pzuraq
Copy link

@pzuraq pzuraq commented Mar 28, 2019

The latest ember-cli-babel supports class fields, so there's no reason to add the plugin here 🎉 and adding it breaks things ☹️


Edit by @chriskrycho, 2019/04/26: For searchability, adding the text of the kind of error users see without this:

WARNING: my-app has added the class-properties plugin to its build, but ember-cli-babel provides these by default now! You can remove the transforms, or the addon that provided them, such as @ember-decorators/babel-transforms.

Copy link
Member

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

Looks good, but we actually have a test covering this, which is currently failing! Do you mind digging in and making sure we get the right behavior?

yarn.lock Outdated
@@ -40,6 +40,17 @@
source-map "^0.5.0"
trim-right "^1.0.1"

"@babel/generator@^7.4.0":
version "7.4.0"
resolved "http://artifactory.corp.linkedin.com:8081/artifactory/api/npm/npm-external-new/@babel/generator/-/@babel/generator-7.4.0.tgz#c230e79589ae7a729fd4631b9ded4dc220418196"
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing this domain won't resolve from the outside world?

Copy link
Author

Choose a reason for hiding this comment

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

ah lol, I guess something got messed up with my config

@josemarluedke
Copy link
Contributor

I can confirm that there are issues with the current version of ember-cli-typescript (v2) when using ember-cli-babel v7.7.

Hopefully this fixes these.

@pzuraq pzuraq force-pushed the bugfix/remove-class-properties-transform-for-latest-babel branch from 267f4e1 to a80bc7e Compare March 29, 2019 21:32
@chriskrycho
Copy link
Member

@pzuraq would you be so kind as to rebase this and one of us will merge it tomorrow? Thanks again!

Copy link
Contributor

@mike-north mike-north left a comment

Choose a reason for hiding this comment

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

We currently have "ember-cli-babel@7.7.3" on master, and no sign of test failure in any environment.

If we can get some steps to reproduce the problem this PR fixes, I'd be happy to implement equivalent tests.

@pzuraq
Copy link
Author

pzuraq commented Apr 2, 2019

Master likely still causes warnings to pop up, so this PR should be updated to not add the class fields plugin and prevent those warnings. Will do it momentarily

@mike-north
Copy link
Contributor

Master likely still causes warnings to pop up, so this PR should be updated to not add the class fields plugin and prevent those warnings. Will do it momentarily

We have tests that assert on terminal output, so it should be possible to write one for this

@chriskrycho
Copy link
Member

@pzuraq mind updating this with the suggested tests (one of us can point you where you need for reference if need be) and a fresh rebase?

@pzuraq
Copy link
Author

pzuraq commented Apr 10, 2019

Yeah, I'm just a bit overloaded at the moment, so unsure when I'll be able to get to updating/adding tests. I can definitely rebase though.

@pzuraq pzuraq force-pushed the bugfix/remove-class-properties-transform-for-latest-babel branch from a80bc7e to 5d81ee3 Compare April 10, 2019 23:51
@chriskrycho
Copy link
Member

Totally understand – thank you!

@chriskrycho
Copy link
Member

Got the warning message today, so now I have a better idea of what we're dealing with here

WARNING: @snap-doc/app has added the decorators plugin to its build, but ember-cli-babel provides these by default now! You can remove the transforms, or the addon that provided them, such as @ember-decorators/babel-transforms. Ember supports the stage 1 decorator spec and transforms, so if you were using stage 2, you'll need to ensure that your decorators are compatible, or convert them to stage 1.

WARNING: @snap-doc/app has added the class-properties plugin to its build, but ember-cli-babel provides these by default now! You can remove the transforms, or the addon that provided them, such as @ember-decorators/babel-transforms.

Originally posted by @mike-north in #623 (comment)

@rwjblue
Copy link
Contributor

rwjblue commented May 2, 2019

Where are we at on this?

@chriskrycho
Copy link
Member

@rwjblue apparently @pzuraq has been buried in other things, and I've been neck-deep in work on Notion, so haven't touched it. I'm blocking out some time to clean up this and a few other things on Monday, though.

Chris Garrett and others added 2 commits May 6, 2019 08:48
@chriskrycho chriskrycho force-pushed the bugfix/remove-class-properties-transform-for-latest-babel branch from 9eac93d to ab19f8a Compare May 6, 2019 14:48
@jamescdavis
Copy link
Member

Closing in favor of #730

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.

7 participants