Fixes for use in Ember 5.x but needs a little help (Issue #110)

[lupestro]

This PR includes a working fix for Ember 5.x, but the test suite needed a bunch of work. I bumped the ember-data and ember-cli-mirage versions used by the dummy app to ones that will work all the way from 3.28 to canary, but no version of ember-data supports both Ember 3.24 and Ember 5.x.

In this drop, the only application test fails, but all other tests pass. The root issue is deep in ember-cli-mirage initialization.

ember-cli-mirage uses @embroider/macros#isTesting to determine whether we are in tests. If we aren't in tests, the app initializer creates the mirage server. If we are in tests, it doesn't and lets setupMirage() do the work. Unfortunately, when we are running tests, the isTesting() embroider macro is returning false during app initialization. So the code tries to start the mirage server and blows up trying to read the application.__container__, which doesn't exist yet. When running the app (perhaps because autoboot is on?) the application.__container__ is there when it hits that initializer.

So here's where I need guidance:

• I'm not sure what needs to happen to address that ember-cli-mirage issue. It seems a shame to drop the only acceptance test in the suite in order to get a fix for a real problem we are encountering out the door. Is that the right thing to do?
• There's no reason the delivered code won't work fine with Ember 3.24, but the test suite cannot test it. Do we drop the scenario from the suite? Do we officially drop support for Ember 3.24 in the addon?
• Can we treat any test suite issues with embroider as a future improvement? Do we drop those scenarios? Or can we make failure of those scenarios non-gating for delivering a new release?

[gorner]

I have also been interested in getting a version of ember-classic-decorator that supports Ember 5. I looked into this PR a bit tonight and couldn't get much (if any) further than the PR author. I did notice there's a vendor file added by Embroider which updates the isTesting value which, in my attempts to run test suite, was being evaluated after the Embroider macro runtime checks for updater methods.

In the test suite for the Ember app I primarily work on, the two functions are run in the correct order allowing Mirage to boot, but I wasn't able to figure out the difference in code that would explain the difference in order, since both are essentially using the same standard tests/index.html, test-helper.js, etc. from the ember-cli blueprint.

Ultimately I don't think this test issue should be a blocking concern for the main fixes proposed here, but regardless I hope someone on the core team can look at cutting a new Ember 5-compatible version soon. Not sure who to ping here other than perhaps @ef4?

Or is this add-on being silently deprecated and we should try to remove it? Searching for updates on the Ember Discord I did see at least one core team member suggest not using @classic at all because lint rules are enough, which may be a defensible position. (The decorator is, however, still a transitive dependency of a couple of the other add-ons we use, so it would not be easy for us to remove it from our build entirely.)

[alechirsch]

Would love to see this go through. Just ran into this as well and we have a lot of classic components

[gzurbach]

Same here. We can't upgrade to Ember 5 because we're using ember-simple-auth which is still using the old syntax.

Updating ember-simple-auth will take some time, so having this fix in the meantime would be incredibly helpful!

[jahrock]

Same here

[gorner]

FWIW I've been able to work around this and upgrade to Ember 5 by specifying this PR branch directly in the overrides block of package.json (assuming you're using NPM as your package manager; with Yarn I believe you can use the resolutions block instead). If you're using it as a direct dependency you'd also need to replace the entry in dependencies or devDependencies as applicable.

e.g.:

"overrides": {
  "ember-classic-decorator": "lupestro/ember-classic-decorator#location-fix-ember53"
}

It's not ideal – merging and cutting a new release would be better; the next best option might be for @lupestro to publish a fork with these changes – but it works for now.

[NullVoxPopuli]

Hugest apologize, i don't know why i clicked the wrong button. But because ci hadn't ran, i reverted in #113 .

Could you re-submit this pr so ci runs? Thank you!

[mansona]

Just to let you all know that I re-opened this in #119 with most of the test-specific issues fixed (I think)

If we can get this finished and over the line I think that would be great 👍