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

Not working on Ember 5 #110

Open
boris-petrov opened this issue May 16, 2023 · 5 comments
Open

Not working on Ember 5 #110

boris-petrov opened this issue May 16, 2023 · 5 comments

Comments

@boris-petrov
Copy link
Contributor

Check this comment.

Otherwise, just add ember-classic-decorator to a brand new Ember 5 app and watch it break.

@BradBarnich
Copy link

I get Uncaught TypeError: Invalid value used as weak map key at http://localhost:7357/assets/vendor.js on https://github.com/emberjs/ember-classic-decorator/blob/master/vendor/classic-decorator/index.js#L25-L26 and https://github.com/emberjs/ember-classic-decorator/blob/master/vendor/classic-decorator/index.js#L40

I was able to get past it by just commenting them out: BradBarnich@4287dce

@lupestro
Copy link

lupestro commented Jan 9, 2024

I forked to do a PR, applied my changes, and immediately ran into a problem running the tests.

  • The tests use ember-data 4.1, which won't run in Ember 5.x because ember-data 4.1 refers to @ember/error, which is deprecated after Ember 4.10 and goes away in 5.0.
  • If I bump the ember-data version higher, to 4.12 or 5.3, I start getting errors from @embroider/macros for babel. I also note that this is for the node_modules that is under ember-cli-mirage, so mirage is somehow perhaps involved. Hence, this is getting waaaay beyond anything I understand how to even begin to troubleshoot.
  • Logistically, ember-data versions that will support 5.x aren't cleared for Ember 3.24, which this addon expects to support, so I'm not sure what we want to sacrifice. I suspect that makes whatever we do a major version bump.

So I have code with the fix and I'll be glad to do the PR, but it is going to take some advice from the maintainers about which version of ember-data we want the testing to use and then how to troubleshoot issues like:

Build Error (broccoli-persistent-filter:Babel > [Babel: @embroider/macros]) in @embroider/macros/runtime.js

/Users/ralphmack/Code/ember/ember-classic-decorator/@embroider/macros/runtime.js: @babel/template placeholder "DEBUG": Expected string substitution

I'm already figuring to add Ember 4.12 LTS and 5.4 LTS to the ember-try scenarios, and have bumped the ember-source version in the package.json on my branch to 4.12.

@LucasHillDex
Copy link

@ef4 do you know if there are any plans to get this addon working with ember 5? We are experiencing this same issue.

@lupestro
Copy link

lupestro commented Jan 16, 2024

I've spent a few days with this and I have the basis for a PR. I have the fix itself - had it the first hour - but it's been an uphill struggle getting the test suite to run. I've bumped ember-data and ember-cli-mirage and worked most of the problems getting the tests to pass.

Using ember-data 4.11 and the latest ember-cli-mirage in the tests:

  • The dummy app is running fine.
  • From 3.28 to canary, I'm down to one failing test, the only application test. This is due to a problem deep in ember-cli-mirage initialization - it's getting @embroider/macros isTesting() returning false when called during app initialization for tests, so there's either an issue with isTesting() or it needs to do something more sophisticated to determine that. (It's trying to defer to initializing the mirage server per test vs. doing it globally when running as an app.)
  • Three ember-try scenarios fail globally: the tests no longer work with ember-lts-3.24 (because there is no version of ember-data that supports both 3.24 and 5.x), and I don't think they were even working before with embroider-safe and embroider-optimized.

If I bump to ember-data 4.12, one additional test fails - it looks like something changes the weak map values for EmberObject from true to false, so it doesn't throw an expected exception in a case where it should.

I'm going to need some help to finish this up. Should I make the PR so we have a place to work from? I can deliver the code from my fork today. Once the test suite can pass CI, it should be ready to deliver.

@lupestro
Copy link

I delivered the PR. Guidance gratefully accepted.

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

No branches or pull requests

4 participants