-
-
Notifications
You must be signed in to change notification settings - Fork 130
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
Upgrade to Ember 5 and convert to a v2 addon #470
Upgrade to Ember 5 and convert to a v2 addon #470
Conversation
Ping @snewcomer any chance we can have checks running for this PR? (and if they are passing, to have this PR merged) Ember 5 is coming and we will need some changes in this project, I believe this PR is a good start (and in fact may be enough to have the compatibility with Ember 5) |
Hi @snewcomer & @hhff |
Hi @jahrock - it's been a long time since I worked on Ember. @snewcomer - what do you think? |
Hi @hhff The effort is quite low and can be achieved through the linked process. |
Yes, absolutely. Let me look into this and I'll get back to you!
…On Tue, Apr 2, 2024 at 3:17 PM Jah Rock ***@***.***> wrote:
Hi @hhff <https://github.com/hhff>
Thanks for your feedback.
Would it be an option for you to transfer the addon ownership to "adopted
ember addons" so that interested developers can get involved?
The effort is quite low and can be achieved through the linked process.
https://github.com/adopted-ember-addons/program-guidelines/blob/master/README.md
—
Reply to this email directly, view it on GitHub
<#470 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAV3R7CHLSD2W45PZNKEMQDY3L76JAVCNFSM6AAAAAAYZELAGWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZSHA4TGNZQGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
*Hugh Francis*
c/o Sanctuary Computer, Inc
120 Walker, FL3
New York, NY 10013
sanctuary.computer <https://www.sanctuary.computer>
|
Hi @hhff |
I am rwwagner90 on npm |
Hi @wozny1989 |
7d22122
to
7fe91b3
Compare
@jahrock done 👍 |
Looks like we need to update the node version used to allow these tests to run |
@RobbieTheWagner I just updated it, let's check |
@wozny1989 shouldn't we also update the ember-try scenarios to include 4.x versions, and beta and canary and such? I think we should ember-cli-update and do whatever the blueprints do there probably. |
@RobbieTheWagner Now it should be fine, tested locally as well: |
package-lock.json
Outdated
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.
Hi @wozny1989
I did not expect that package-lock.json
and yarn.lock
to be present.
Is this on purpose?
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.
@jahrock My bad, just removed package-lock.json
👍
7f09747
to
ef229ad
Compare
@wozny1989 it looks like we need to update node versions. Can you update engines in package.json and our ci.yml to enforce node 18+ please? |
@RobbieTheWagner Done 👍 |
tests/.eslintrc.js
Outdated
@@ -1,9 +1,9 @@ | |||
/*eslint-env node*/ |
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 this file should be deleted, as the root .eslintrc.js handles configuring the test file linting now
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.
Right, I just deleted it
@RobbieTheWagner Also I bumped |
On my fork CI have different output, but why? wozny1989#1 |
@RobbieTheWagner Finally I migrated addon to more embroider friendly addon v2, please re-run CI and let's keep finger crossed 🤞 fyi @jahrock |
A v2 addon is definitely what we want for the future, but this is quite a lot to review now 😅 |
As I see, many addons get ported from npm to pnpm. Would it make sense to give it a try? |
Migrate to pnpm
@RobbieTheWagner Sorry for that! I had no other idea how to fix the tests on Embroider 😅
@jahrock I just migrated to |
Bump to Ember 5
Finally I bumped to Ember 5, now we have support for Ember >= 3.28.x |
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.
Works as expected within my ember 4.12 app.
There are no further suggestions from my side.
.github/workflows/ci.yml
Outdated
run: echo "::set-output name=dir::$(yarn cache dir)" | ||
try-scenario: | ||
- ember-lts-3.24 | ||
- ember-lts-3.28 |
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.
Hi @wozny1989
As suggested by @RobbieTheWagner we should test all 4.x LTS versions to ensure complatibility.
- ember-lts-3.28 | |
- ember-lts-3.28 | |
- ember-lts-4.4 | |
- ember-lts-4.8 | |
- ember-lts-4.12 |
config/ember-try.js
Outdated
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.
Hi @wozny1989
Let's add the 4.+ LTS versions here as well, so we can verify locally that all versions work as expected.
@jahrock Thanks for review, just added scenarios for LTS 4.12 and 5.8 👍 |
- ember-lts-5.4 | ||
- ember-lts-5.8 |
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 noticed that 5.8 was just released and it's current state is LTS candidate
, until 5.9 get released.
Ember released 5.8
IMHO it should be ok to add the next LTS version in advance.
Hi @RobbieTheWagner |
I've noticed a few issues with this but I'll fix it after merging since it's such a large PR to review 🙈 |
Tasks
content
or passed custom params: