-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
UI: Ember 3.0 -> 3.4 #5467
UI: Ember 3.0 -> 3.4 #5467
Conversation
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.
Really easy to review thanks to logically split commits, thank you 🙌
ui/tests/unit/adapters/job-test.js
Outdated
@@ -383,6 +383,10 @@ module('Unit | Adapter | Job', function(hooks) { | |||
const region = 'region-2'; | |||
window.localStorage.nomadActiveRegion = region; | |||
|
|||
// Regions are fetched in the before hook, so manually dirty the activeRegion | |||
// instead of repeating what is in the beforeEach here. | |||
this.system.notifyPropertyChange('activeRegion'); |
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 jumped out at me a bit, and I had to go and read the system service to understand what's going on here. Is there any alternative to notifyPropertyChange
here? For example, might it make sense to call this.system.set('activeRegion', region)
, or are you trying to test that the computed property fetches the region from local storage?
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.
Specifically trying to test that the computed property fetches the region from local storage.
The alternative is to get namespaces after setting the localStorage property, but that means all my URL checks need to include namespaces, or omit the first entry.
This is not my favorite but it made for the smallest diff and least repetitive assertions. Maybe it's still worth refactoring.
@alisdair, I decided to refactor those unit tests to avoid the |
@DingoEatingFuzz Nice! More explicit and definitely clearer to me 😃 |
ui/config/deprecation-workflow.js
Outdated
@@ -5,6 +5,8 @@ self.deprecationWorkflow.config = { | |||
{ handler: 'throw', matchId: 'ember-inflector.globals' }, | |||
{ handler: 'throw', matchId: 'ember-runtime.deprecate-copy-copyable' }, | |||
{ handler: 'throw', matchId: 'ember-console.deprecate-logger' }, | |||
{ handler: 'silence', matchId: 'ember-component.send-action' }, |
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.
TIL about this file
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.
From this addon! https://github.com/mixonic/ember-cli-deprecation-workflow
Super helpful for triaging deprecations. I don't think you're supposed to commit it like this, but I'm gonna try it out. My main fear is deprecations related to str.pluralize()
creeping back in. This makes it a runtime error now.
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Officially on the latest LTS 🎉
The bulk of the work here was making tests pass since some things mostly in Ember Data changed. Additionally, I addressed the immediate deprecations, opened a PR in ivy-codemirror, and ran the ES5 Getters codemod.