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

Require ember-resources v7 #687

Merged
merged 7 commits into from
May 24, 2024
Merged

Conversation

basz
Copy link
Contributor

@basz basz commented Feb 24, 2024

following these instructions I abstracted the class based resources of e-r 6 to those of ember-resource 7.

NullVoxPopuli/ember-resources#1056

It works in the sense that all tests pass

@cah-brian-gantzler
Copy link
Contributor

Fixes #686

I had a PR as well but it would not let me submit it.

@simonihmig could we get this workflow run please. This is blocking me from updating ember concurrency V4 as ember-resources 6.x has a peer-dependency on ember-concurrency 3.x

Copy link
Owner

@simonihmig simonihmig left a comment

Choose a reason for hiding this comment

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

Thanks for working in this! LGTM, happy to merge once green. There seems to be a minor linting error though.

@basz
Copy link
Contributor Author

basz commented Mar 9, 2024

lint issues resolved

@basz
Copy link
Contributor Author

basz commented Mar 14, 2024

uhg. lots of errors suddenly. I don't have time to dive into these atm...

@cah-brian-gantzler
Copy link
Contributor

Did some experimentation. The first CI works which appears to be using ember 4.0 (and @types/ember_application 4.0.7). When trying other versions of ember, @types/ember_application 4.0.11 is being used which is where the errors are being generated. Appears to be mis-matched typescript types I guess. Hoping the little research I did helps, but very new to typescript and this is beyond my ability.

@cah-brian-gantzler
Copy link
Contributor

Anyone able to take a look at this? It seems to be a problem of the types dependencies and not actually the code changes.

@basz
Copy link
Contributor Author

basz commented Mar 27, 2024 via email

@cah-brian-gantzler
Copy link
Contributor

Anyone have any chance in the foreseeable future? Have multiple addons that can not be updated as a result of this as addons are moving on to require ember-resources V7.

@basz
Copy link
Contributor Author

basz commented May 24, 2024

@simonihmig I've updates some dependencies in the hope we will get a green light. could you rerun the pipeline?

@simonihmig
Copy link
Owner

@basz linting fails with some competing ember type definitions, and also in the yarn.lock you can see different versions of type packages being used. I think that could be solved by running npx yarn-deduplicate hopefully!

@basz
Copy link
Contributor Author

basz commented May 24, 2024

👍 after some fiddling it run locally. Can't do the matrix somehow... lets see how this goes

@simonihmig
Copy link
Owner

There is some new ESLint error due to render-modifiers, probably due to new dependency versions?

But I wonder where did all the @types/ember__* packages go to, don't we need them?

@basz
Copy link
Contributor Author

basz commented May 24, 2024

Saw the error. looking into how to avoid using ember-template-modifiers atm

I kept getting conflicts between types coming from @types/ember and @types/ember__*, so i just removed then to if that would resolve the conflicts. It did. All tests run fine except for the ember-template-modifiers. Let's see if I can fix that.

@simonihmig
Copy link
Owner

looking into how to avoid using ember-template-modifiers atm

I think we might want to just turn off the rule for now. I think we should migrate those to custom modifiers, but that seems well out of scope for this PR! :)

@basz
Copy link
Contributor Author

basz commented May 24, 2024

pretty sure it run green aallll the way now... If you could do a release soon... it's my last unmet peer dependency thank you so much!

@cah-brian-gantzler
Copy link
Contributor

Thank you for completing this. Its blocking some updates for me as well.

@simonihmig simonihmig changed the title Ember resources 7 Require ember-resources v7 May 24, 2024
@simonihmig simonihmig added enhancement New feature or request breaking labels May 24, 2024
@simonihmig simonihmig merged commit 8428a4b into simonihmig:master May 24, 2024
10 checks passed
@simonihmig
Copy link
Owner

Thanks again for working on this @basz!

I think the requirement for a new major version of ember-resources, which many apps and addons also directly depend on, needs to be seen as a breaking change, so also require a new "major" from this addon. This is on 0.5, so that would be 0.6 (kinda indicating a breaking change), but that's no big deal.

I would like to also add some other breaking changes, like dropping support for older Ember versions. At this point 4.12 to start with seems reasonable? Or does anyone of you here have concerns with that? @basz @cah-brian-gantzler

(btw, it's not that we actively prevent those versions from working, just removing from the test matrix and not committing for support anymore)

@cah-brian-gantzler
Copy link
Contributor

I am fine with all that. I agree with the breaking change. Technically any change to a 0.x is considered a possible breaking change. It is not until 1.x that changes to x should not be a breaking change.

@basz
Copy link
Contributor Author

basz commented May 24, 2024

sounds good to me.

@basz
Copy link
Contributor Author

basz commented Jun 3, 2024

hi @simonihmig

What are the other BC you propose and can we help with those?

@simonihmig
Copy link
Owner

Just dropping support for older Ember versions I think. Quickly raised this, if you want to have a look: #696

@simonihmig
Copy link
Owner

@basz
Copy link
Contributor Author

basz commented Jun 3, 2024

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants