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

Support Ember v5 by addin @ember/string to peerDependencies #547

Merged
merged 9 commits into from
Nov 29, 2024

Conversation

jelhan
Copy link
Contributor

@jelhan jelhan commented Nov 19, 2024

This is a breaking change as it is adding @ember/string as an additional peer dependency.

It included #550 which should be merged first.

@jelhan jelhan marked this pull request as draft November 19, 2024 13:21
@jelhan jelhan marked this pull request as ready for review November 19, 2024 14:47
@Windvis
Copy link
Contributor

Windvis commented Nov 19, 2024

This is a breaking change as it is adding @ember/string as an additional peer dependency.

It's just a devDep at the moment, no? Also, @ember/string is a v2 addon so ember-auto-import v2 needs to be a dep I think?

@jelhan
Copy link
Contributor Author

jelhan commented Nov 19, 2024

This is a breaking change as it is adding @ember/string as an additional peer dependency.

It's just a devDep at the moment, no? Also, @ember/string is a v2 addon so ember-auto-import v2 needs to be a dep I think?

You are right. It should have gone into peer dependencies. But somehow I missed it. No idea why the tests are passing anyways here but no in my app. Thanks a lot for careful review! Will fix it.

@jelhan jelhan marked this pull request as draft November 19, 2024 16:51
@jelhan
Copy link
Contributor Author

jelhan commented Nov 20, 2024

I learned that there is a 2.0.0-beta.2 release, which supports Ember v5. The code is not in this repository. But it is available on NPM.

@jelhan jelhan closed this Nov 20, 2024
@jelhan jelhan changed the title Support Ember v4 Support Ember v5 Nov 26, 2024
@jelhan
Copy link
Contributor Author

jelhan commented Nov 26, 2024

The code for 2.0.0-beta.2 is in this repository. This is the tag.

Also 2.0.0-beta.2 is not compatible with Ember v5. It fails due to missing peer dependency on @ember/string.

@jelhan jelhan reopened this Nov 26, 2024
@jelhan jelhan marked this pull request as ready for review November 26, 2024 21:06
@jelhan
Copy link
Contributor Author

jelhan commented Nov 26, 2024

It should work. Also the added test scenario for Ember v5.12 passes. But I haven't tested in a real app yet.

README.md Outdated
@@ -124,7 +124,7 @@ ember install ember-metrics

- Ember.js v3.20 or above
Copy link
Contributor

Choose a reason for hiding this comment

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

as a follow up, we may bump support for 3.28+ which should simplify CI and should not affect many (if any) users as 3.20 -> 3.28 upgrade should be pretty simple

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have done that in #549.

@jelhan jelhan marked this pull request as draft November 29, 2024 09:29
@jelhan
Copy link
Contributor Author

jelhan commented Nov 29, 2024

It seems that some outdated dependencies weren't compatible with Ember v5 causing an error. After I merged in #550 it works fine. So let's land that upgrade first and than add the support for Ember v5 in the next step.

@SergeAstapov
Copy link
Contributor

@jelhan mind to rebase this PR as #550 landed

@jelhan jelhan marked this pull request as ready for review November 29, 2024 14:09
@@ -59,6 +59,7 @@ jobs:
try-scenario:
- ember-lts-3.28
- ember-lts-4.12
Copy link
Contributor

Choose a reason for hiding this comment

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

@jelhan shouldn't we add the whole matrix of LTS versions in v4 and v5, like 4.4, 4.8, 5.4, 5.8?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a realistic risk that it works for 4.12 and 5.12 but not for interim releases? CI minutes are free for open source projects on GitHub. But we should nevertheless review the value for each scenario as they have other trade-offs such as environmental impact.

Happy to add if you feel they provide real value.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jelhan very good question and considering ember-source had very little changes during v4 and v5, I guess answer is "no". It's more like a convention I've seen around in the ecosystem and been following for the addons I've tried to maintain.

@SergeAstapov SergeAstapov merged commit cb4ec4f into adopted-ember-addons:master Nov 29, 2024
6 checks passed
@SergeAstapov SergeAstapov changed the title Support Ember v5 Support Ember v5 by addin @ember/string to peerDependencies Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants