-
Notifications
You must be signed in to change notification settings - Fork 43
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
Conversation
83a78bb
to
92ff135
Compare
@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. |
@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. |
Thank you for triggering the tests! 👀 Fixing the 5.3 failures is definitely important. The specific error is:
This might be solved by bumping some deps (either in |
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 |
Does that also fail on 5.3? 🥺 |
Rebased against |
218f10f
to
7e1e966
Compare
So CI fails on Ember 5.3 onwards, stumbling upon
That fails with:
I tried adding Upgrading I have found a combination of We have discussed this with @mansona, @NullVoxPopuli and @void-mAlex. Chris's recommendation is to upgrade the addon itself to latest Ember via @mixonic, I'm gonna do that now. Is that cool? UPD: |
bdbdbf9
to
89e1892
Compare
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. |
There was a problem hiding this comment.
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?
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 |
There's no reason we can't still support that too. The strategy would be: in vendor.js, put a And the registration function should notice whether the vendor.js code successfully installed the deprecation handlers and not stomp on them. |
a50332b
to
f7ef9bd
Compare
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this 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 🎉
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? 🙏 |
blocking? this strategy works great: https://github.com/universal-ember/reactiveweb/blob/main/tests/test-app/app/app.ts#L8-L10 |
@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 |
well, RFC incoming 😅 |
Depends on Upgrade Ember to 3.28 #158, merge that first. While not merged, see this for a cleaner diff.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 avendor/
file and used a funky old way to accessregisterDeprecationHandler
:In order to make this addon Embroider-friendly, this line needs to be converted to:
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
:Config moved from
config/deprecation-workflow.js
toapp/deprecation-workflow.js
, and its format changed. UseflushDeprecations()
to regenerate the content of the config file (see readme).