ember-cli / ember-cli-deprecation-workflow

MIT License
165 stars 43 forks source link

[BREAKING] Convert to a module. Drops support for Ember < 3.28, requires manual initialization #159

Closed lolmaus closed 8 months ago

lolmaus commented 1 year ago

I have rebased this PR on top of #178 (Upgrade Ember CLI to 5.4). Please use this link to see a clean diff. Do not merge until #178 is merged!


ember-cli-deprecation-workflow was written as a vendor/ file and used a funky old way to access registerDeprecationHandler:

let registerDeprecationHandler = require.has('@ember/debug') ? require('@ember/debug').registerDeprecationHandler : Ember.Debug.registerDeprecationHandler;

In order to make this addon Embroider-friendly, this line needs to be converted to:

import { registerDeprecationHandler } from `@ember/debug`;

Such imports don't work in vendor/, so this addon has been converted to a proper module.

Another problem that was interfering with Embroider was that it was heavily relying on Broccoli magic. All Broccoli hooks have been removed, the addon now requires manual initialization from app/app.js.

Breaking changes

lolmaus commented 1 year ago

@mixonic Can you please make sure that no mentions of now-unsupported Ember versions like 3.16 remain in docs and code? 🙏 I might have missed a few.

mixonic commented 1 year ago

@lolmaus I've rebased a bit and updated the CI suite. I think this is probably in a good state for further iteration- I'm not sure we should merge it unless there is a passing suite on 5.3 and canary, while we can drop everything before 3.28.

Do you agree?

I'm going to open a ticket summarizing the goals of a major version release for us to discuss making several breaking changes at once, Ember support version being only one of them.

lolmaus commented 1 year ago

Thank you for triggering the tests! 👀

Fixing the 5.3 failures is definitely important.

The specific error is:

Global error: Uncaught Error: Could not find module @ember/polyfills imported from @ember/test-helpers/-internal/debug-info at http://localhost:7357/assets/vendor.js, line 259

This might be solved by bumping some deps (either in package.json or in the ember-try scenario).

mixonic commented 1 year ago

FYI my best example for an embroiderified addon that supports 3.28 through 5.2 is at https://github.com/html-next/vertical-collection/blob/master/tests/dummy/config/ember-try.js. Note that I used the try scenario to pick out some dependency versions in testing specifically for 3.28. I think if we do that, we can use modern versions in package.json and get things to a good state.

lolmaus commented 1 year ago

through 5.2

Does that also fail on 5.3? 🥺

lolmaus commented 12 months ago

Rebased against master.

lolmaus commented 12 months ago

So CI fails on Ember 5.3 onwards, stumbling upon @ember/polyfills missing.

@ember/polyfills is invoked by old "@ember/test-helpers": "2.2.9", so I tried bumping it in ember-try scenarios.

That fails with:

errorMessage: ember-qunit has the following unmet peerDependencies:

  • @ember/test-helpers: ^2.1.0; it was resolved to 3.2.1

I tried adding overrides, but that fails too.

Upgrading ember-qunit does not seem possible because latest QUnit does not define a global, which trips Testem. 🙈

I have found a combination of @ember/test-helpers and ember-qunit versions that does build, but on Ember 5.3 the test suite starts failing for some reason.

We have discussed this with @mansona, @nullvoxpopuli and @void-malex. Chris's recommendation is to upgrade the addon itself to latest Ember via ember-cli-update in a separate PR, get it merged, and then rebase this PR on top of latest Ember.

@mixonic, I'm gonna do that now. Is that cool? UPD: done. 😅 https://github.com/mixonic/ember-cli-deprecation-workflow/pull/178

lolmaus commented 12 months ago

⚠️ I have rebased this PR on top of #178 (Upgrade Ember CLI to 5.4). Please use this link to see a clean diff. Do not merge until #178 is merged! 🙏

mixonic commented 10 months ago

ember-cli-deprecation-workflow was written as a vendor/ file and used a funky old way to access registerDeprecationHandler:

Wanted to note that the reason this was done was to make the addon capable of controlling deprecations during initial payload evaluation and template compilation. The template compilation part already was removed in a prior release, but the app.js based setup may still miss deprecations from addons which are not Embroider addons and may have evaluation time deprecated API usage in vendor.js. Regardless, I think those cases are steadily decreasing and there are other advantages to using a more traditional/normal setup style 👍

ef4 commented 10 months ago

capable of controlling deprecations during initial payload evaluation and template compilation

There's no reason we can't still support that too. The strategy would be: in vendor.js, put a try around require(...) of ember stuff. If it fails, that's fine because when Ember is not accessible from vendor, nobody in vendor can use Ember's deprecated API anyway. I think it would still be good in this case to tell users to do the manual registration in app.js too, so the instructions are consistent and they're prepared for the upgrade to strict ES modules.

And the registration function should notice whether the vendor.js code successfully installed the deprecation handlers and not stomp on them.

lolmaus commented 9 months ago

@mixonic @ef4 I have applied your suggestion.

I have also ensured the addon works in both normal and Embroider-strict apps. Turns out, the readme guide was incomplete, so I updated it, creating two sets of instructions.

CC @mansona.

simonihmig commented 8 months ago

Where are we with this? Would love to be able to use this, as this is blocking Embroider adoption for us currently.

@mixonic would you be able to give this a review? :pray:

NullVoxPopuli commented 8 months ago

as this is blocking Embroider adoption for us currently.

blocking? this strategy works great: https://github.com/universal-ember/reactiveweb/blob/main/tests/test-app/app/app.ts#L8-L10

mansona commented 8 months ago

@NullVoxPopuli I can very much believe that this is blocking large orgs, and I know a few people who are indeed blocked by this so let's focus on getting it over the line instead of suggesting that we all hand-roll replacement solutions 👍

@simonihmig I'll add this to the agenda for Tuesday, this isn't in the ember-cli org yet but I know that @ef4 has merge rights so we can get this moved forward

NullVoxPopuli commented 8 months ago

instead of suggesting that we all hand-roll replacement solutions

well, RFC incoming :sweat_smile: