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

Class private methods are not enabled. #419

Closed
NullVoxPopuli opened this issue Nov 11, 2021 · 50 comments · Fixed by #420
Closed

Class private methods are not enabled. #419

NullVoxPopuli opened this issue Nov 11, 2021 · 50 comments · Fixed by #420

Comments

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Nov 11, 2021

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/[email protected]
│ ├─┬ @ember/[email protected]
│ │ └── [email protected] deduped
│ ├── [email protected] deduped
│ └─┬ [email protected]
│   └── [email protected] deduped
├─┬ @glimmer/[email protected]
│ └── [email protected] deduped
├─┬ [email protected]
│ ├─┬ @embroider/[email protected]
│ │ └─┬ @embroider/[email protected]
│ │   └── [email protected] deduped
│ └── [email protected] deduped
├── [email protected]
├─┬ [email protected]
│ └── [email protected] deduped
├─┬ [email protected]
│ └── [email protected] deduped
├─┬ [email protected]
│ └── [email protected] deduped
├─┬ [email protected]
│ └── [email protected]
├─┬ [email protected]
│ └── [email protected] deduped
├─┬ [email protected]
│ ├── [email protected] deduped
│ └─┬ [email protected]
│   └── [email protected] deduped
├─┬ [email protected]
│ └── [email protected] deduped
├─┬ [email protected]
│ └── [email protected] deduped
├─┬ [email protected]
│ └── [email protected] deduped
├─┬ [email protected]
│ ├── [email protected] deduped
│ └─┬ [email protected]
│   └── [email protected] deduped
└─┬ [email protected]
  └── [email protected] deduped


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

  origin  [email protected]: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
Copy link

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: tracked-tools/tracked-built-ins#204

@NullVoxPopuli
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

SergeAstapov commented Nov 13, 2021

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

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

@NullVoxPopuli
Copy link
Contributor Author

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
Copy link
Member

rwjblue commented Nov 16, 2021

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
Copy link
Contributor Author

on it

@chriskrycho
Copy link
Contributor

@NullVoxPopuli I'm already pushing a repro repo.

NullVoxPopuli added a commit to NullVoxPopuli/repro-for-emberjs-19790 that referenced this issue Nov 16, 2021
@NullVoxPopuli
Copy link
Contributor Author

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

@chriskrycho
Copy link
Contributor

chriskrycho commented Nov 16, 2021

Lulz. Here's mine. Same.

@NullVoxPopuli
Copy link
Contributor Author

Lulz. Here's mine. Same.

image

🙌

@chriskrycho
Copy link
Contributor

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
Copy link
Member

rwjblue commented Nov 17, 2021

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
Copy link
Member

rwjblue commented Nov 17, 2021

Looks like [email protected]+ will have the newer version (landed in emberjs/data@325ce04), but not in 3.24/3.28 just yet.

chriskrycho added a commit to tracked-tools/tracked-built-ins that referenced this issue Nov 18, 2021
This gets us on the relevant latest versions of *all* of our
dependencies, transitive and otherwise, including the latest versions of
`@babel/preset-env` and `ember-cli`, which together trigger the build
error seen in #204 and emberjs/ember-cli-babel#419.

**This commit fails intentionally.** The follow-on commit will fix it,
and the CI history will thereby act equivalent to a fixed failing test.
@NullVoxPopuli
Copy link
Contributor Author

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

@vmurillo
Copy link

hello, is there any workaround for this problem?

@NullVoxPopuli
Copy link
Contributor Author

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

@NullVoxPopuli
Copy link
Contributor Author

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
Copy link

nickschot commented Nov 25, 2021

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: #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: #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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

NullVoxPopuli commented Nov 26, 2021

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
Copy link
Contributor Author

NullVoxPopuli commented Nov 26, 2021

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: 0f1b995

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. 😢

@kategengler
Copy link
Member

@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
Copy link

vjonesmuth commented Dec 1, 2021

I am writing a generic typescript package using [email protected] that is being consumed by an ember app using [email protected]. 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
Copy link
Contributor Author

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: 0f1b995#diff-4d3a8edb07aff5a7c43d8692de88a4eb83b327e2a352633699ba4afb9b753b6aR21

🤷

@NullVoxPopuli
Copy link
Contributor Author

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

@vjonesmuth
Copy link

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

NullVoxPopuli added a commit to NullVoxPopuli/ember-statechart-component that referenced this issue Dec 3, 2021
Thanks to emberjs/ember-cli-babel#419 (comment)
from @VinceJones, tests run again, and this work is no longer blocked by
emberjs/ember-cli-babel#419

BREAKING CHANGE: this package now requires ember-auto-import@v2
@NullVoxPopuli
Copy link
Contributor Author

Thanks, @VinceJones !!

I confirm that in NullVoxPopuli/ember-statechart-component#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. 🎉

@NullVoxPopuli
Copy link
Contributor Author

@NullVoxPopuli
Copy link
Contributor Author

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

@NullVoxPopuli
Copy link
Contributor Author

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

github-actions bot pushed a commit to NullVoxPopuli/ember-statechart-component that referenced this issue Dec 5, 2021
# [3.0.0](v2.3.7...v3.0.0) (2021-12-05)

### Bug Fixes

* **deps:** update dependency tracked-maps-and-sets to v3 ([40618e9](40618e9))

### chore

* drop support for node 12 ([b1b3c4c](b1b3c4c))
* **ci:** update test matrix ([91de3bb](91de3bb))
* **deps:** pin @babel/helper-create-class-features-plugin ([1a8111b](1a8111b)), closes [/github.com/emberjs/ember-cli-babel/issues/419#issuecomment-985735309](https://github.com//github.com/babel/ember-cli-babel/issues/419/issues/issuecomment-985735309)

### BREAKING CHANGES

* node 12 is no longer supported
* **ci:** drop support for ember 3.26
* **deps:** this package now requires ember-auto-import@v2
@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Dec 5, 2021

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

NullVoxPopuli added a commit to NullVoxPopuli/limber that referenced this issue Dec 6, 2021
ember-cli-babel ruins lives :(
(or maybe just babel)

Blocked by: NullVoxPopuli/ember-statechart-component#149
which is in-turn blocked by: emberjs/ember-cli-babel#419
which is then blocked by ember-data... *for some reason*.
(I don't use ember-data here, so idgaf)

Further work is blocked by embroider-build/embroider#1038
@chriskrycho
Copy link
Contributor

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.)

chriskrycho added a commit to chriskrycho/ember-cli-babel that referenced this issue Dec 7, 2021
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 pushed a commit to chriskrycho/ember-cli-babel that referenced this issue Dec 8, 2021
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 pushed a commit to chriskrycho/ember-cli-babel that referenced this issue Dec 8, 2021
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
@BryanCrotaz
Copy link

BryanCrotaz commented Oct 31, 2022

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

@NullVoxPopuli
Copy link
Contributor Author

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

@nickschot
Copy link

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: #442

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

@BryanCrotaz
Copy link

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/[email protected]
└─┬ ember-bootstrap
  ├── @embroider/[email protected]
  └─┬ ember-element-helper
    └── @embroider/[email protected]

tracked-toolbox
Allowed: (any single version)
Found: 1.2.3, 2.0.0
ashleigh-app-frontend
├── [email protected]
├─┬ ember-bootstrap
│ └── [email protected]
└─┬ ember-tracked-effects-placeholder
  └── [email protected]

@pzubar
Copy link

pzubar commented Jan 23, 2023

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
Copy link

I went through and upgraded all my addons.

@minwyy
Copy link

minwyy commented Feb 17, 2023

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.

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 a pull request may close this issue.