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

Update ember-cli to v4.12 #736

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Update ember-cli to v4.12 #736

wants to merge 3 commits into from

Conversation

fsmanuel
Copy link
Contributor

@fsmanuel fsmanuel commented Oct 25, 2023

Follow up on #732

  • Updates ember-cli to v4.12
  • Updates the tests to work with ember-cli v4.12
  • Tries to import Model from @ember-data/model to detect if ember-data is installed
  • Removes a isArray on ember-data objects deprecation
  • Increases ember-data test coverage to v4.11 - v4.12 is a breaking change because ember-data v4.12 requires node >= 16 and the addon requires >= 14
  • Removed obsolete config files

Release, Beta and Canary are expected to fail. Current test coverage is 3.28 -> 4.12

I think we should release this as v5.0.1 because it removes some deprecations and reduces the consuming apps noise.

#716

@fsmanuel fsmanuel self-assigned this Oct 25, 2023
@fsmanuel fsmanuel requested a review from a team October 26, 2023 12:22
@fsmanuel fsmanuel mentioned this pull request Oct 26, 2023
14 tasks
Copy link
Contributor

@gilest gilest left a comment

Choose a reason for hiding this comment

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

Nice. Looks generally like a good approach.

Release target

I think we should release this as v5.0.1 because it removes some deprecations and reduces the consuming apps noise.

Aren't there already unreleased breaking changes (node v12 dropped since 5.0)?

My suggestion would be to also drop Node 16 and cut a major to avoid more rounds of breaking changes.

Also a general suggestion to try and separate changes into more PRs. I know it's hard when no one is reviewing and there's lots of deferred maintenance like in this addon <3

Embroider

Question about the embroider suite failures – do you think they're genuine or to do with ember try and test app configuration?

Reason I ask is that I'm aware of a few apps building and running file using ember-cp-validations v5.

@fsmanuel
Copy link
Contributor Author

fsmanuel commented Oct 29, 2023

@gilest tanks for the review!

Aren't there already unreleased breaking changes (node v12 dropped since 5.0)?

You are right! I missed that we haven't released #731

My suggestion would be to also drop Node 16 and cut a major to avoid more rounds of breaking changes.

That sounds to be right. I think #738 is a good place to drop Node v14.

Embroider

Question about the embroider suite failures – do you think they're genuine or to do with ember try and test app configuration?

I opened #739 and #740 to track the use of ember and ember-data internals. I suspect it to be the cause of the embroider failures.

Reason I ask is that I'm aware of a few apps building and running file using ember-cp-validations v5.

With v5 you mean ember v5?

Also a general suggestion to try and separate changes into more PRs. I know it's hard when no one is reviewing and there's lots of deferred maintenance like in this addon <3

You are absolutely right! So much better for the changelog and in general. I'm already confused what is released or not...

@gilest
Copy link
Contributor

gilest commented Nov 5, 2023

With v5 you mean ember v5?

I meant ember-cp-validations v5.

What I was reporting is that it's running fine with ember 4.12 and full embroider.

I'm not sure if that's even useful as I've forgotten the context for this PR 😅

@fsmanuel
Copy link
Contributor Author

fsmanuel commented Nov 8, 2023

@mansona I rebased and this is what is left.

@mansona
Copy link
Contributor

mansona commented Nov 8, 2023

So I don't think we need any more of this PR so I think we can just close it 🤔 I opened all the other PRs with your work because they were all good and I didn't want you to have to go through the pain of rebasing after I changed things from under you 🙈. What's left here doesn't really help with anything, and the change that you make to import @ember-data/model would require that to become an option peer dependency so if we do want to go down that route then we should open a PR to discuss it there 👍

I made an attempt to fix embroider stuff and it's getting us a lot further: #749 🎉 We have a problem though 😞 it seems like you can't define computed properties the way that we're doing it here in the tests: https://github.com/adopted-ember-addons/ember-cp-validations/blob/master/tests/integration/validations/factory-general-test.js#L498

I don't know why this sort of issue would be unearthed by using Embroider and not by an ember upgrade 🤔 And unfortunately I don't have any idea how to change the test to get it to pass. Any thoughts?

@fsmanuel
Copy link
Contributor Author

fsmanuel commented Nov 9, 2023

@mansona I think it is related to #739

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.

3 participants