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

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

Merged
merged 8 commits into from Feb 27, 2024

Conversation

lolmaus
Copy link
Contributor

@lolmaus lolmaus commented Oct 23, 2023

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

  • Drops support for Ember versions earlier than 3.28.

  • Requies manual initialization in app/app.js:

    import from './deprecation-workflow';
  • Config moved from config/deprecation-workflow.js to app/deprecation-workflow.js, and its format changed. Use flushDeprecations() to regenerate the content of the config file (see readme).

@lolmaus
Copy link
Contributor Author

lolmaus commented Nov 1, 2023

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

mixonic commented Nov 1, 2023

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

lolmaus commented Nov 1, 2023

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

mixonic commented Nov 1, 2023

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

lolmaus commented Nov 1, 2023

through 5.2

Does that also fail on 5.3? 🥺

@lolmaus
Copy link
Contributor Author

lolmaus commented Nov 22, 2023

Rebased against master.

@lolmaus lolmaus force-pushed the ember-debug branch 7 times, most recently from 218f10f to 7e1e966 Compare November 22, 2023 15:00
@lolmaus
Copy link
Contributor Author

lolmaus commented Nov 22, 2023

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. 😅 #178

@lolmaus
Copy link
Contributor Author

lolmaus commented Nov 23, 2023

⚠️ 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! 🙏

@lolmaus lolmaus force-pushed the ember-debug branch 2 times, most recently from bdbdbf9 to 89e1892 Compare November 23, 2023 11:10
README.md Outdated
5. Copy the string output into `config/deprecation-workflow.js` in your project.
3. Run your test suite\* with `ember test --server`.
4. Navigate to your tests (default: http://localhost:7357/)
5. Run `deprecationWorkflow.flushDeprecations()` from your browsers console.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the new setup that requires manual installation of depreciation workflow, when does that setup happen? If it comes from an post-install blueprint, can that also be explicitly documented here?

README.md Outdated Show resolved Hide resolved
@mixonic
Copy link
Owner

mixonic commented Dec 24, 2023

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

ef4 commented Jan 5, 2024

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 lolmaus force-pushed the ember-debug branch 4 times, most recently from a50332b to f7ef9bd Compare January 25, 2024 12:00
@lolmaus
Copy link
Contributor Author

lolmaus commented Jan 25, 2024

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

README.md Outdated
@@ -43,18 +43,43 @@ addressing a single deprecation at a time, and prevents backsliding

### Getting started

The initial steps needed to get started:
#### Normal setup routine
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should only communicate one setup routine, if the embroider one works fine for classic let's just do that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link

@mansona mansona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me 🎉

@simonihmig
Copy link

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? 🙏

@NullVoxPopuli
Copy link

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

mansona commented Feb 22, 2024

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

instead of suggesting that we all hand-roll replacement solutions

well, RFC incoming 😅

@ef4 ef4 marked this pull request as ready for review February 27, 2024 16:30
@ef4 ef4 merged commit 0b4cbe7 into mixonic:master Feb 27, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants