-
Notifications
You must be signed in to change notification settings - Fork 50
Update ember-stargate to 0.5.0 #2840
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git βοΈ
|
A couple of questions:
|
A lot of these seem to be on much older versions like 0.2.0, so are already not in sync with the version we have here. This is another one we could fix with a peerDep and allow apps to install their own versions, if that is what we want. The only breaking change in ember-stargate 0.5.0 is it requires Ember 3.24+, so anything on older Ember than that would have problems, but it looks like HDS only supports Ember 3.28+ already, is that correct?
The 3.28 tests seem to be passing, so I do not think it is a problem. |
EmbroiderUtilRegistry /*, other addon registries */ { | ||
// local entries | ||
LinkToExternal: typeof LinkTo; | ||
// we have to use `ember-stargate` version `0.4.3` because version `0.5.0` causes a break in the tests for `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.
This seems like an important reason to not make this change RN
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'm not sure if it is correct though. The tests seem to be passing.
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.
Tests may pass in our addon but not in consuming apps. However, if you've tested this in a consuming app that uses 3.28 and found that it still works, please update the comment to be correct but still leave it in. Thanks!
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.
Gotcha, I can mark this as release candidate and try it in an app on 3.28. Assuming it all works fine, what would you like the comment to say?
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 installed this in nomad and all tests are still passing there as well hashicorp/nomad#25750
I would propose we remove the comment completely, as I don't think it provides any relevant details at this time, but if there is a new comment you would like there, I would be happy to add it, just let me know!
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.
@MelSumner if the tests are passing, there's no need anymore of the comment (I was the one that added it, and at that time the tests were not passing).
@zamoore can you have a look also at this PR? thanks |
π¦ RC Packages PublishedLatest commit: ad897d1 |
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.
My concerns have been addressed, thank you!
π Summary
This PR updates ember-stargate to 0.5.0.
π οΈ Detailed description
This allows updating ember-stargate and ember-resources in consuming apps and gets us on newer versions to allow us to continue iterating toward latest. While I was making this change, I noticed we had some webpack peerDeps missing, so I also installed those.
πΈ Screenshots
π External links
Jira ticket: HDS-XXX
Figma file: [if it applies]
π Component checklist
π¬ Please consider using conventional comments when reviewing this PR.