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

[FEAT] Decorator and Class Field Support #274

Merged
merged 4 commits into from
Mar 27, 2019

Conversation

pzuraq
Copy link
Collaborator

@pzuraq pzuraq commented Mar 16, 2019

This PR adds the decorator and class field transforms directly to the
stardard Ember build, so users can rely on ember-cli-babel to provide
them and won't have to manage their own transforms package. It does
not add these transforms if it detects that the plugins have already
been added by something else, and instead warns.

This PR adds the decorator and class field transforms directly to the
stardard Ember build, so users can rely on `ember-cli-babel` to provide
them and won't have to manage their own transforms package. It does
_not_ add these transforms if it detects that the plugins have already
been added by something else, and instead warns.
@pzuraq pzuraq force-pushed the feat/decorator-and-class-field-support branch from 766f277 to 520190f Compare March 20, 2019 16:50
index.js Outdated
) {
this.project.ui.writeWarnLine(`${
this._parentName()
} has added the decorators and/or class properties plugins 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.`);
Copy link
Member

Choose a reason for hiding this comment

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

I am slightly worried by line-noise cause by this warning, i have some questions:

  • how often does this fire for a large app (with many nested ember-cli-babel's)?
  • can it be reduced further, and maybe more meaty details to a readme?
  • it should likely surface what version of ember-cli-babel now provides this via
  • where does a user remove the decorators from, their add-on or their app or...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Those are good points. Do you think we should only warn if it's a top level thing? So, apps will warn, and addons will only warn if they are being worked on/developed. I've found this a good way to move the ecosystem forward in the past when things aren't incredibly pressing, and I think this qualifies since the transforms do tend to be limited to a single package.

@stefanpenner
Copy link
Member

stefanpenner commented Mar 25, 2019

I left a note, re: warning. But otherwise 👍 .

Question, for non-decorator users how much does this slow down the users babel?

@pzuraq
Copy link
Collaborator Author

pzuraq commented Mar 25, 2019

I haven't seen any noticeable changes in build-speed, but I also haven't used the decorator transforms in a large application. Is there a way we can benchmark to find out what the change would be?

@stefanpenner
Copy link
Member

I haven't seen any noticeable changes in build-speed, but I also haven't used the decorator transforms in a large application. Is there a way we can benchmark to find out what the change would be?

Super low tech, is something like: rm -rf $TMPDIR/$USER && time ember b
Although the above will be relatively low fidelity, we are really just looking for large regressions, as anything minor is outweighed by the value of this pr.

@pzuraq pzuraq force-pushed the feat/decorator-and-class-field-support branch from ec4a47f to 80e42a4 Compare March 27, 2019 15:14
@pzuraq
Copy link
Collaborator Author

pzuraq commented Mar 27, 2019

I benchmarked and shared results with @stefanpenner offline, this does not appear to be a regression performance-wise. I've also updated the warning to only log in the root application/addon, so users won't get large amounts of noise from addons they don't control.

I think this should be good to go!

@rwjblue rwjblue dismissed stefanpenner’s stale review March 27, 2019 15:32

warning issue addressed

@rwjblue rwjblue merged commit 591de2b into emberjs:master Mar 27, 2019
@rwjblue
Copy link
Member

rwjblue commented Mar 27, 2019

ember-cli-babel 7.7.0 published 🎉

@stefanpenner
Copy link
Member

Thanks for the extra diligence

@LordCHTsai
Copy link

LordCHTsai commented Mar 28, 2019

Ran into an issue when reinstalling dependencies within my project. ember-cli@3.7.0 requires ember-cli-babel@^7.6.0 so it installed 7.7.0 because of the ^ symbol. Then I guess because of some of the subdependencies use decorators-legacy, I got the error

Build Error (broccoli-persistent-filter:Babel > [Babel: ember-data]) in ember-data/adapters/errors.js

Cannot use the decorators and decorators-legacy plugin together

disableDecoratorTransforms: true doesn't seem to work to me.

@pzuraq
Copy link
Collaborator Author

pzuraq commented Mar 28, 2019

Hey @LordCHTsai, can you possibly provide a reproduction? It seems strange that the plugin would be included twice, definitely would like to get to the bottom of it.

@LordCHTsai
Copy link

@pzuraq here's the reproduction
npm install -g ember@3.7.0 (also confirm 3.7.1 also fails, but 3.8.1 works fine)

 >  ember version
ember-cli: 3.7.1
node: 10.15.1
os: darwin x64
> ember new ember-quickstart-ya
> cd ember-quickstart-ya
> npm install
> ember serve
Build Error (broccoli-persistent-filter:Babel > [Babel: ember-data]) in ember-data/-debug/index.js

Cannot use the decorators and decorators-legacy plugin together


Stack Trace and Error Report: /var/folders/v1/4g8cyy3j0hj485fyx7h964h8001336/T/error.dump.f67baab64d5e9d29c0174e55f465b659.log

and here's the log

=================================================================================

ENV Summary:

  TIME: Thu Mar 28 2019 10:18:12 GMT-0700 (Pacific Daylight Time)
  TITLE: ember
  ARGV:
  - /Users/chetsai/.nvm/versions/node/v10.15.1/bin/node
  - /Users/chetsai/.nvm/versions/node/v10.15.1/bin/ember
  - serve
  EXEC_PATH: /Users/chetsai/.nvm/versions/node/v10.15.1/bin/node
  TMPDIR: /var/folders/v1/4g8cyy3j0hj485fyx7h964h8001336/T
  SHELL: /bin/bash
  PATH:
  - /Users/chetsai/.nvm/versions/node/v10.15.1/bin
  - /Users/chetsai/.pyenv/shims
  - /usr/local/bin
  - /usr/local/sbin
  - /usr/bin
  - /bin
  - /usr/sbin
  - /sbin
  - /usr/local/linkedin/bin
  - /export/content/linkedin/bin
  PLATFORM: darwin x64
  FREEMEM: 9791586304
  TOTALMEM: 34359738368
  UPTIME: 603078
  LOADAVG: 7.89990234375,4.79345703125,4.330078125
  CPUS:
  - Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz - 2900
  - Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz - 2900
  - Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz - 2900
  - Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz - 2900
  - Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz - 2900
  - Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz - 2900
  - Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz - 2900
  - Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz - 2900
  - Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz - 2900
  - Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz - 2900
  - Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz - 2900
  - Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz - 2900
  ENDIANNESS: LE
  VERSIONS:
  - ares: 1.15.0
  - cldr: 33.1
  - http_parser: 2.8.0
  - icu: 62.1
  - modules: 64
  - napi: 3
  - nghttp2: 1.34.0
  - node: 10.15.1
  - openssl: 1.1.0j
  - tz: 2018e
  - unicode: 11.0
  - uv: 1.23.2
  - v8: 6.8.275.32-node.12
  - zlib: 1.2.11

ERROR Summary:

  - broccoliBuilderErrorStack: Error: Cannot use the decorators and decorators-legacy plugin together
    at validatePlugins (/Users/chetsai/Work/test-ember-3/ember-quickstart-ya/node_modules/@babel/parser/lib/index.js:11085:13)
    at getParser (/Users/chetsai/Work/test-ember-3/ember-quickstart-ya/node_modules/@babel/parser/lib/index.js:11150:5)
    at parse (/Users/chetsai/Work/test-ember-3/ember-quickstart-ya/node_modules/@babel/parser/lib/index.js:11133:12)
    at parser (/Users/chetsai/Work/test-ember-3/ember-quickstart-ya/node_modules/@babel/core/lib/transformation/normalize-file.js:170:34)
    at normalizeFile (/Users/chetsai/Work/test-ember-3/ember-quickstart-ya/node_modules/@babel/core/lib/transformation/normalize-file.js:138:11)
    at runSync (/Users/chetsai/Work/test-ember-3/ember-quickstart-ya/node_modules/@babel/core/lib/transformation/index.js:44:43)
    at transformSync (/Users/chetsai/Work/test-ember-3/ember-quickstart-ya/node_modules/@babel/core/lib/transform.js:43:38)
    at Object.transform (/Users/chetsai/Work/test-ember-3/ember-quickstart-ya/node_modules/@babel/core/lib/transform.js:22:38)
    at resolve (/Users/chetsai/Work/test-ember-3/ember-quickstart-ya/node_modules/ember-cli-babel/node_modules/broccoli-babel-transpiler/lib/worker.js:11:29)
    at initializePromise (/Users/chetsai/Work/test-ember-3/ember-quickstart-ya/node_modules/ember-cli-babel/node_modules/rsvp/dist/rsvp.js:520:5)
  - code: [undefined]
  - codeFrame: Cannot use the decorators and decorators-legacy plugin together
  - errorMessage: ember-data/-debug/index.js: Cannot use the decorators and decorators-legacy plugin together
        in /var/folders/v1/4g8cyy3j0hj485fyx7h964h8001336/T/broccoli-67954wuxWwsX56Bsd/out-076-funnel
        at broccoli-persistent-filter:Babel > [Babel: ember-data] (Babel: ember-data)
  - errorType: Build Error
  - location:
    - column: [undefined]
    - file: ember-data/-debug/index.js
    - line: [undefined]
    - treeDir: /var/folders/v1/4g8cyy3j0hj485fyx7h964h8001336/T/broccoli-67954wuxWwsX56Bsd/out-076-funnel
  - message: ember-data/-debug/index.js: Cannot use the decorators and decorators-legacy plugin together
        in /var/folders/v1/4g8cyy3j0hj485fyx7h964h8001336/T/broccoli-67954wuxWwsX56Bsd/out-076-funnel
        at broccoli-persistent-filter:Babel > [Babel: ember-data] (Babel: ember-data)
  - name: BuildError
  - nodeAnnotation: Babel: ember-data
  - nodeName: broccoli-persistent-filter:Babel > [Babel: ember-data]
  - originalErrorMessage: Cannot use the decorators and decorators-legacy plugin together
  - stack: Error: Cannot use the decorators and decorators-legacy plugin together
    at validatePlugins (/Users/chetsai/Work/test-ember-3/ember-quickstart-ya/node_modules/@babel/parser/lib/index.js:11085:13)
    at getParser (/Users/chetsai/Work/test-ember-3/ember-quickstart-ya/node_modules/@babel/parser/lib/index.js:11150:5)
    at parse (/Users/chetsai/Work/test-ember-3/ember-quickstart-ya/node_modules/@babel/parser/lib/index.js:11133:12)
    at parser (/Users/chetsai/Work/test-ember-3/ember-quickstart-ya/node_modules/@babel/core/lib/transformation/normalize-file.js:170:34)
    at normalizeFile (/Users/chetsai/Work/test-ember-3/ember-quickstart-ya/node_modules/@babel/core/lib/transformation/normalize-file.js:138:11)
    at runSync (/Users/chetsai/Work/test-ember-3/ember-quickstart-ya/node_modules/@babel/core/lib/transformation/index.js:44:43)
    at transformSync (/Users/chetsai/Work/test-ember-3/ember-quickstart-ya/node_modules/@babel/core/lib/transform.js:43:38)
    at Object.transform (/Users/chetsai/Work/test-ember-3/ember-quickstart-ya/node_modules/@babel/core/lib/transform.js:22:38)
    at resolve (/Users/chetsai/Work/test-ember-3/ember-quickstart-ya/node_modules/ember-cli-babel/node_modules/broccoli-babel-transpiler/lib/worker.js:11:29)
    at initializePromise (/Users/chetsai/Work/test-ember-3/ember-quickstart-ya/node_modules/ember-cli-babel/node_modules/rsvp/dist/rsvp.js:520:5)

=================================================================================

@LordCHTsai
Copy link

Update:
By looking into the log, it seems like it happens when building ember-data, so I tried to bump the ember-data version to 3.8.0 and keep ember-cli version the same 3.7.0, it actually works.

At least it's narrowing down to ember-data. I will keep digging into the reason.

@simonihmig
Copy link
Contributor

simonihmig commented Mar 28, 2019

Just ran into this as well. Created a new app with Ember CLI 3.7 and ran ember s. Downgrading ember-cli-babel to 7.6.0 (using yarn resolutions) also fixed this!

@pzuraq
Copy link
Collaborator Author

pzuraq commented Mar 28, 2019

@rwjblue and I tracked it down this morning: #275

TL;DR: babel-plugin-filter-imports was doing bad things ☹️

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

Successfully merging this pull request may close these issues.

5 participants