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 to ember 5.12 #807

Merged
merged 6 commits into from
Jan 20, 2025
Merged

Update to ember 5.12 #807

merged 6 commits into from
Jan 20, 2025

Conversation

piemonkey
Copy link
Contributor

Overview

Update packages to align with the blueprint, fix the linting, but don't fix the deprecations. Some deprecation fixes will follow in a later PR.

connected issues and PRs:

Editor PR: lblod/ember-rdfa-editor#1237
Plugins PR: lblod/ember-rdfa-editor-lblod-plugins#528
Jira ticket: https://binnenland.atlassian.net/browse/GN-5329

Setup

N/A

How to test/reproduce

Check that everything works!

Challenges/uncertainties

Without updating ember-data, had to keep an older @ember/string as well as adding an @ember-data/store dep to prevent npm from trying to install v5.

Checks PR readiness

  • UI: works on smaller screen sizes
  • UI: feedback for any loading/error states
  • Check cancel/go-back flows
  • Check database state correct when deleting/updating (especially regarding relationships)
  • changelog
  • npm lint
  • no new deprecations

@piemonkey piemonkey requested review from lagartoverde, abeforgit and elpoelma and removed request for lagartoverde and abeforgit January 14, 2025 08:53
Copy link
Member

@abeforgit abeforgit left a comment

Choose a reason for hiding this comment

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

can't find any problems, looks good to me!

Copy link
Contributor

@elpoelma elpoelma left a comment

Choose a reason for hiding this comment

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

Found one issue (related to the usage of the removed replaceRoute method defined on controllers).
Relevant issue: emberjs/ember.js#19497
(this.router.replaceWith fires the model hook unnecessarily)
We could introduce the suggested solution: emberjs/ember.js#19497 (comment)

@piemonkey
Copy link
Contributor Author

Found one issue (related to the usage of the removed replaceRoute method defined on controllers). Relevant issue: emberjs/ember.js#19497 (this.router.replaceWith fires the model hook unnecessarily) We could introduce the suggested solution: emberjs/ember.js#19497 (comment)

Hmm, I wasn't sure about replacing this. It was on my list of 'longer to fix ember 5 deprecations', one of the few that I didn't get to. What I was afraid of is that since we were already using replaceRoute(), we might be accidentally relying on this behaviour, so replacing it might break things. We definitely should fix it, with careful testing. Do you think it blocks this PR though?

@elpoelma
Copy link
Contributor

Found one issue (related to the usage of the removed replaceRoute method defined on controllers). Relevant issue: emberjs/ember.js#19497 (this.router.replaceWith fires the model hook unnecessarily) We could introduce the suggested solution: emberjs/ember.js#19497 (comment)

Hmm, I wasn't sure about replacing this. It was on my list of 'longer to fix ember 5 deprecations', one of the few that I didn't get to. What I was afraid of is that since we were already using replaceRoute(), we might be accidentally relying on this behaviour, so replacing it might break things. We definitely should fix it, with careful testing. Do you think it blocks this PR though?

I think the main issue is that the method has been removed in ember.js 5+, so it simply no longer works.

@piemonkey
Copy link
Contributor Author

I think the main issue is that the method has been removed in ember.js 5+, so it simply no longer works.

Ah, then yes, we definitely should fix it...

Added a hack around a longstanding bug in RoutingService to make the
replacement behave as expected
@piemonkey piemonkey requested a review from elpoelma January 20, 2025 15:41
@piemonkey piemonkey merged commit 6bf4044 into master Jan 20, 2025
3 checks passed
@piemonkey piemonkey deleted the internal/ember-5 branch January 20, 2025 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants