emberjs / ember-cli-babel

Ember CLI plugin for Babel
MIT License
153 stars 119 forks source link

Class private methods are not enabled. #419

Closed NullVoxPopuli closed 2 years ago

NullVoxPopuli commented 2 years ago

I just upgraded tracked-built-ins in an addon and am getting this error:

Build Error (broccoli-persistent-filter:Babel > [Babel: tracked-built-ins]) in tracked-built-ins/-private/object.js

ember-statechart-component/tracked-built-ins/-private/object.js: Class private methods are not enabled.
  65 |
  66 |   // @private
> 67 |   #readStorageFor(key) {
     |   ^
  68 |     let storage = this.#storages.get(key);
  69 |
  70 |     if (storage === undefined) {

but, what's weird, is that I've worked on tracked-built-ins with private methods... and this wasn't an issue at all.

My ember-cli-babel appears up to date:

❯ npm ls ember-cli-babel
├─┬ @ember/test-helpers@2.6.0
│ ├─┬ @ember/test-waiters@3.0.0
│ │ └── ember-cli-babel@7.26.6 deduped
│ ├── ember-cli-babel@7.26.6 deduped
│ └─┬ ember-destroyable-polyfill@2.0.3
│   └── ember-cli-babel@7.26.6 deduped
├─┬ @glimmer/component@1.0.4
│ └── ember-cli-babel@7.26.6 deduped
├─┬ ember-auto-import@1.12.0
│ ├─┬ @embroider/core@0.33.0
│ │ └─┬ @embroider/macros@0.33.0
│ │   └── ember-cli-babel@7.26.6 deduped
│ └── ember-cli-babel@7.26.6 deduped
├── ember-cli-babel@7.26.6
├─┬ ember-cli-typescript-blueprints@3.0.0
│ └── ember-cli-babel@7.26.6 deduped
├─┬ ember-could-get-used-to-this@1.0.1
│ └── ember-cli-babel@7.26.6 deduped
├─┬ ember-load-initializers@2.1.2
│ └── ember-cli-babel@7.26.6 deduped
├─┬ ember-maybe-import-regenerator@0.1.6
│ └── ember-cli-babel@6.18.0
├─┬ ember-page-title@6.2.2
│ └── ember-cli-babel@7.26.6 deduped
├─┬ ember-qunit@5.1.5
│ ├── ember-cli-babel@7.26.6 deduped
│ └─┬ ember-cli-test-loader@3.0.0
│   └── ember-cli-babel@7.26.6 deduped
├─┬ ember-resolver@8.0.3
│ └── ember-cli-babel@7.26.6 deduped
├─┬ ember-source@3.26.2
│ └── ember-cli-babel@7.26.6 deduped
├─┬ qunit-dom@1.6.0
│ └── ember-cli-babel@7.26.6 deduped
├─┬ tracked-built-ins@2.0.0
│ ├── ember-cli-babel@7.26.6 deduped
│ └─┬ ember-tracked-storage-polyfill@1.0.0
│   └── ember-cli-babel@7.26.6 deduped
└─┬ tracked-maps-and-sets@3.0.2
  └── ember-cli-babel@7.26.6 deduped

ember-statechart-component on  upgrade-debs [$!] 

  origin  git@github.com:NullVoxPopuli/ember-statechart-component.git 

❯ latestPkgVer ember-cli-babel
Current:
"7.26.6"

Tags:
{
  "latest": "7.26.6",
  "alpha": "7.0.0-alpha.2",
  "beta": "7.24.0-beta.2",
  "5.x": "5.2.8",
  "old": "6.17.2"
}

Most Recent:
"7.26.6"

and browserslist seems to be up to date as well.

❯ npx browserslist --update-db
Latest version:     1.0.30001279
Installed version:  1.0.30001279
caniuse-lite is up to date
caniuse-lite has been successfully updated

No target browser changes
nightire commented 2 years ago

I've seen it recently after I upgraded tracked-built-ins to v2. I have no idea what causes this error, so I raised an issue here: https://github.com/tracked-tools/tracked-built-ins/issues/204

NullVoxPopuli commented 2 years ago

that's the library where am seeing this, too.

It's not tracked-built-ins' fault tho. We should be able to use private properties and methods in our apps -- if we do, we get the same error.

NullVoxPopuli commented 2 years ago

oh! I just tested this, that's not true!!! private methods work just fine in the host app.

ok, so this bug with ember-cli-babel is that addons are not appropriately inheriting the browserlist or targets? 🤔 maybe?

SergeAstapov commented 2 years ago

Another option, smith with TS compilation as add-on source is .ts

EDIT: I may be wrong as there are .js files

NullVoxPopuli commented 2 years ago

tracked-built-ins is a V1 addon, so TS compilation happens as a result of the app telling the addon to compile... which.. is handled by the host's ember-cli-babel 🤔 I think ember-cli-babel has a misconfiguration for addons 🤔

rwjblue commented 2 years ago

Is there a minimal reproduction for this? Can someone give me a repo to clone (with either a lockfile that demonstrates the issue, or checked in node_modules)?

NullVoxPopuli commented 2 years ago

on it

chriskrycho commented 2 years ago

@NullVoxPopuli I'm already pushing a repro repo.

NullVoxPopuli commented 2 years ago

Here is my repro: https://github.com/NullVoxPopuli/repro-for-emberjs-19790/pull/2 (branch: ember-cli-babel-issues-419) all you need to do is yarn && yarn build

chriskrycho commented 2 years ago

Lulz. Here's mine. Same.

NullVoxPopuli commented 2 years ago

Lulz. Here's mine. Same.

image

:raised_hands:

chriskrycho commented 2 years ago

Additional info to make things extra complicated: we’re now consuming latest v. of tracked-built-ins in the big app at LinkedIn, with no issues. And our build is not doing anything to configure private class fields (as you would expect given the status of that proposal!). Notably, both our app and the repro apps are all resolving ember-cli-babel 7.26.6, so it's unlikely it's ember-cli-babel itself. I will continue reporting as I have time.

rwjblue commented 2 years ago

Debugged this with @chriskrycho a bit. The main issue (we believe) is that ember-cli-babel is always adding @babel/plugin-proposal-class-properties (which we added originally for decorators support):

https://github.com/babel/ember-cli-babel/blob/4c3b9091d7c711ecb804a52226409b409a702d82/lib/babel-options-util.js#L345-L359

Path forward in this repo is to change that guard above to only add the class-properties plugin when it is needed (possibly by using isPluginRequired).

Unfortunately, we also need to land some fixes to ember-data LTS versions. Specifically, the version of rollup they use in their internal build infrastructure does not support class fields, so when ember-cli-babel stops automatically adding that plugin ember-data's build fails like:

- broccoliBuilderErrorStack: Error: Unexpected token
    at error (/Users/chris/dev/test/yoooooo/node_modules/rollup/dist/shared/node-entry.js:5400:30)
    at Module.error (/Users/chris/dev/test/yoooooo/node_modules/rollup/dist/shared/node-entry.js:9824:16)
    at tryParse (/Users/chris/dev/test/yoooooo/node_modules/rollup/dist/shared/node-entry.js:9717:23)

At the moment that is using rollup@^1.12.0, via these packages:

https://github.com/emberjs/data/blob/362ae1af977b9a4ca328cc0aafc45b87e6041b91/packages/private-build-infra/package.json#L27

https://github.com/chadhietala/broccoli-rollup/blob/c45027c53e0b72c5f06158e3efed67cc63f4ef8f/package.json#L53

tldr; we need to bump @ember-data/-private-build-infra to use broccoli-rollup@5 (which allows us to upgrade to latest rollup, which should support the syntax we need).

rwjblue commented 2 years ago

Looks like ember-data@4.0.0-beta.1+ will have the newer version (landed in https://github.com/emberjs/data/commit/325ce04ea5f14b72bac270711ab600dfe085ac46), but not in 3.24/3.28 just yet.

NullVoxPopuli commented 2 years ago

looks like this happens with optional chaining as well (from v2 addons)

vmurillo commented 2 years ago

hello, is there any workaround for this problem?

NullVoxPopuli commented 2 years ago

not that I know of -- we have to fix ember-cli-babel

NullVoxPopuli commented 2 years ago

I attempted this: https://github.com/babel/ember-cli-babel/compare/master...NullVoxPopuli:idk?expand=1

but I get errors from within babel with the isPluginRequired method:

Cannot read property 'firefox' of undefined
nickschot commented 2 years ago

I've also run into issues with this. Forcefully including the plugin in ember-cli-build then results in an issue caused by an outdated pinned version of @babel/runtime(7.12) which is missing @babel/runtime/helpers/esm/classPrivateMethodInitSpec. See: https://github.com/babel/ember-cli-babel/pull/385

Forcing a resolution allowing newer runtimes solves this issue but triggers another issue Could not find module "@babel/runtime/helpers/esm/checkPrivateRedeclaration.js" imported from "@babel/runtime/helpers/esm/classPrivateMethodInitSpec". Turning off externalized helpers "fixes" that one too.... but that's of course not great. This is probably caused by: https://github.com/babel/ember-cli-babel/issues/384 ?

Not sure what the way forward is, but I assume having a pinned version @babel/runtime doesn't help things either?

edit: fyi, pinning "@babel/helper-create-class-features-plugin": "7.14.8" also "fixes" the automatic include of class private methods. So something in the newer versions of that plugin must have influenced things.

chriskrycho commented 2 years ago

If you read @rwjblue's explanation above, the things you're seeing will make sense: it's basically because the transitives of those earlier values end up including earlier versions of the database which preset-env uses to determine which plugins are required. The problem, most briefly, is that for it to work correctly, all of the class fields plugins are required together if any of them are required, and ember-cli-babel was only adding one of them—we really just want to not add any of them automatically (the reasons we were doing that historically are no longer relevant), but dropping them broke Ember Data which was accidentally implicitly relying on what ember-cli-babel was doing.

The “fix” here is: don't update dependencies which are causing this breakage until we get out releases of ember-cli-babel and Ember Data which resolve the issue. Which is a bit slowed because of (a) the Ember 4 releases and (b) this week being American Thanksgiving! 🦃 🥧 🇺🇸 (Given which I'm going to vanish back into non-OSS land now. 😉)

NullVoxPopuli commented 2 years ago

What should those of us who don't use ember-data do? Is there PR we can point at, etc?

Kind of a shame for ember data to be such a blocker. 🤔 (At least for all the non-ember-data folks)

NullVoxPopuli commented 2 years ago

It's just this babel issue is holding up a lot of work for me, and I don't like that something I don't use is blocking :(

oh, I just saw @nickschot's comment -- gonna try some pinning so I can at least do something. :D


Update: no pinning works. I guess my only option now is to ditch the addon@v1 format entirely and pre-compile my stuff via addon@v2, and add tracked-built-ins to my test app 🤔

NullVoxPopuli commented 2 years ago

This is immensely frustrating -- pretty much can't work atm. :(

Here is my last attempt, where I just started deleting a bunch of the legacy crap: https://github.com/babel/ember-cli-babel/commit/0f1b9954802278f637e98daf4f74677d3892b4d0

but, the overall problem remains where the host app's targets aren't considered when running babel:

Build Error (broccoli-persistent-filter:Babel > [Babel: @ember/test-helpers]) in @ember/test-helpers/-internal/debug-info.js

/NullVoxPopuli/ember-statechart-component/@ember/test-helpers/-internal/debug-info.js: Missing class properties transform.
  40 |
  41 | export class TestDebugInfo {
> 42 |   _summaryInfo = undefined;
     |   ^^^^^^^^^^^^^^^^^^^^^^^^^
  43 |
  44 |   constructor(settledState, debugInfo = getDebugInfo()) {
  45 |     this._settledState = settledState;

I know it's more efficient to only have each addon's needs use specific plugins, as each additional plugin can add compile time, but this has fealt very brittle, and I feel lost / frustrated / upset. 😢

chriskrycho commented 2 years ago

This is indeed why v2 addons will be better, and I sympathize with the frustrations.

A friendly reminder, though: a bunch of maintainers are on vacation this week, and time off is good and important for everyone! We'll get back to this next week. 😄

snewcomer commented 2 years ago

Looks like ember-data@4.0.0-beta.1+ will have the newer version (landed in emberjs/data@325ce04), but not in 3.24/3.28 just yet.

In 3.28.5!

chriskrycho commented 2 years ago

All right, cool. I'll chat with @rwjblue and see if we can land the fix in ember-cli-babel now that Data is unblocked, and that should then cascade out to the rest of the ecosystem "automagically" by way of Renovate/Dependabot/etc.

NullVoxPopuli commented 2 years ago

Was the fix already figured out, but kept secret?

chriskrycho commented 2 years ago

It's what we already said upthread!

NullVoxPopuli commented 2 years ago

where?

kategengler commented 2 years ago

@NullVoxPopuli The fix was described https://github.com/babel/ember-cli-babel/issues/419#issuecomment-979372114 and https://github.com/babel/ember-cli-babel/issues/419#issuecomment-971787156

NullVoxPopuli commented 2 years ago

it seems ember-data focused (which I'm not using, and my work isn't using either), and nothing I tried following that guidance got me success :shrug:

It's just a bit too high level for someone unfamiliar with ember-cli-babel :upside_down_face:

kategengler commented 2 years ago

@NullVoxPopuli I promise I have no subject-matter expertise here. Reading above:

The problem, most briefly, is that for it to work correctly, all of the class fields plugins are required together if any of them are required, and ember-cli-babel was only adding one of them—we really just want to not add any of them automatically (the reasons we were doing that historically are no longer relevant), but dropping them broke Ember Data which was accidentally implicitly relying on what ember-cli-babel was doing.

My reading here is that the fix is to not add any of the class field plugins in e-c-babel, but that fix was blocked by a fix to Ember Data because the e-c-babel fix broke Ember Data.

vjonesmuth commented 2 years ago

I am writing a generic typescript package using rollup@2.60.0 that is being consumed by an ember app using ember-cli-babel@7.26.6. When I run ember s I see the error described Class private methods are not enabled. Do we just need to wait for ember-cli-babel to push out the fix or is there a work around that I can implement? I tried pinning broccoli-rollup to 5.0.0 but that didn't seem to help.

NullVoxPopuli commented 2 years ago

My reading here is that the fix is to not add any of the class field plugins in e-c-babel

I tried that here, with excludes and no success: https://github.com/babel/ember-cli-babel/commit/0f1b9954802278f637e98daf4f74677d3892b4d0#diff-4d3a8edb07aff5a7c43d8692de88a4eb83b327e2a352633699ba4afb9b753b6aR21

:shrug:

NullVoxPopuli commented 2 years ago

@rwjblue or @chriskrycho are you able to put up a PR with the fix?

vjonesmuth commented 2 years ago

Pinning @babel/helper-create-class-features-plugin to 7.14.6 seems to have fixed this for me

NullVoxPopuli commented 2 years ago

Thanks, @VinceJones !!

I confirm that in https://github.com/NullVoxPopuli/ember-statechart-component/pull/149 pinning @babel/helper-create-class-features-plugin to 7.14.6 via https://www.npmjs.com/package/force-resolutions (with npm) allows me to run tests, and unblock a month's worth of C.I. :tada:

NullVoxPopuli commented 2 years ago

but it doesn't help with embroider :thinking: https://github.com/NullVoxPopuli/ember-statechart-component/runs/4412492851?check_suite_focus=true

NullVoxPopuli commented 2 years ago

it seems there is no way to make resolutions work with ember-try + npm -- gonna try switching to yarn

NullVoxPopuli commented 2 years ago

It's no wonder I'm having so many issues, these issue numbers are anagrams of each other! image

NullVoxPopuli commented 2 years ago

Related:

NullVoxPopuli commented 2 years ago

in ember-statechart-component, I was able to get passed this issue by... not using tracked-built-ins. (I instead went for the tracked-storage-primitives-polyfill, which is probably the better thing to do for this addon anyway). So, that at least unblocks some of my open source stuff.

I still consider this issue to be an ecosystem-wide outage tho

chriskrycho commented 2 years ago

rwjblue and I are going to try to get this knocked out tomorrow; in the meantime, folks may be able to work around it by pinning caniuse-lite to 1.0.30001280. (We did that for an internal addon and it did the trick.)

BryanCrotaz commented 1 year ago

I'm still seeing this with Ember 4.3.0 with npm pdfjs-dist

NullVoxPopuli commented 1 year ago

Do you have the latest ember-cli-babel, and only one copy in your whole package graph?

nickschot commented 1 year ago

From what I've seen this issue can still occur in certain situations when a newer than 7.12.x @babel/runtime sneaks up to the root of node_modules and you have externalHelpers enabled. See: https://github.com/babel/ember-cli-babel/issues/442

Is this what you're seeing too @BryanCrotaz or is there another path to breakage?

BryanCrotaz commented 1 year ago

This is what I see:

ember dependency-lint
@embroider/util
Allowed: (any single version)
Found: 1.9.0, 1.6.0
ashleigh-app-frontend
├─┬ @ember/test-helpers
│ └── @embroider/util@1.9.0
└─┬ ember-bootstrap
  ├── @embroider/util@1.6.0
  └─┬ ember-element-helper
    └── @embroider/util@1.6.0

tracked-toolbox
Allowed: (any single version)
Found: 1.2.3, 2.0.0
ashleigh-app-frontend
├── tracked-toolbox@2.0.0
├─┬ ember-bootstrap
│ └── tracked-toolbox@1.2.3
└─┬ ember-tracked-effects-placeholder
  └── tracked-toolbox@1.2.3
pzubar commented 1 year ago

Hello @BryanCrotaz! I face the same issue with pdfjs-dist integration. Did you manage to find a fix (other than using the "legacy" version)? Could you please share your findings? Thank you in advance!

BryanCrotaz commented 1 year ago

I went through and upgraded all my addons.

minwyy commented 1 year ago

I'm still seeing this with Ember 4.3.0 with npm pdfjs-dist

Hi @BryanCrotaz , I am doing the pdfjs-dist integration and meeting the same issue. I realised it is actually ember-auto-import issue as imported node module package (e.g. pdfjs) are transpired, instead of by ember-cli-babel, are by "babel-loader" with webpack when being imported by ember-auto-import. It should be working out-of-box as private methods are officially supported in babel/preset-env, however there is a bug preventing that. After I manually implement the babel-loader in the ember-cli-build the integration is working now. Here is my test app.