-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 upgrade: Handle deprecation router service from host #28603
UI: Ember upgrade: Handle deprecation router service from host #28603
Conversation
Build Results: |
CI Results: |
@@ -8,20 +8,27 @@ import Resolver from 'ember-resolver'; | |||
import loadInitializers from 'ember-load-initializers'; | |||
import config from 'vault/config/environment'; | |||
|
|||
// TODO: DEPRECATION https://ember-engines.com/docs/deprecations#-use-alias-for-inject-router-service-from-host-application |
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.
now engines reference the alias app-router
instead of router
directly
@@ -23,7 +23,7 @@ import { isAfter } from 'date-fns'; | |||
*/ | |||
|
|||
export default class MessagesList extends Component { | |||
@service router; | |||
@service('app-router') router; |
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 is how you pass the service if the variable you assign it to (i.e. router
) differs from the service name
<MyForm @onSave={{transition-to "vault.cluster.some.route.item" "item-id"}} /> | ||
<MyForm @onSave={{transition-to "vault.cluster.some.external.route" external=true}} /> | ||
*/ | ||
export default class TransitionTo extends Helper { |
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.
Open to other ideas here, this seemed like a nice and simple way to dynamically use the transition-to
helper without knowing the router ahead of time. It also was nice to remove an dependency 🎉
For reference, Chelsea had made a specific engine-transition-to
helper here. Which is what I used as a basis for building this component.
Also there are no instances now where we pass external=true
but I've copied her test to validate this works as expected.
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.
I like this approach a lot. Nice work!
@@ -136,7 +136,6 @@ | |||
"ember-qunit": "^8.0.1", | |||
"ember-resolver": "^11.0.1", | |||
"ember-responsive": "5.0.0", | |||
"ember-router-helpers": "^0.4.0", |
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.
replaced with (new) global transition-to
helper
@@ -4,7 +4,7 @@ | |||
*/ | |||
|
|||
import Component from '@glimmer/component'; | |||
import { service } from '@ember/service'; | |||
import { getOwner } from '@ember/owner'; |
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.
👍🏻
@@ -11,7 +11,6 @@ import { task } from 'ember-concurrency'; | |||
|
|||
export default Mixin.create({ | |||
store: service(), | |||
router: service(), |
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.
I wonder why we had this service in here to begin with🤔
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.
Yeah, I was going to move the service to the components that used the ReplicationActions
mixin but then realized this wasn't used anywhere in the mixin or where the mixin was used?? My guess is left behind during a refactor?
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.
But would definitely appreciate a second set of eyes double checking!
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.
I was able to smoke test replication and other transitions. Nice work!
Description
Handles Use alias for inject router service from host application. This PR is based on initial work done by #28281, which I recommend comparing to the changes here.
In summary this PR:
router
engine dependencies with aliasedapp-router
@service('app-router') router
within engines instead of@service router
Deviations from original PR:
transition-to
slightly differently than the original PR. I uninstalled ember-router-helpers and replaced it with a globaltransition-to
helper that usesgetOwner
to dynamically determine which router to use.LinkedBlock
(which is a core component used across engines and the main app) to lookup the appropriate router service via a getter, which felt more stable instead of attempting to inject a service that doesn't exist (which according to above, would throw an error)TODO only if you're a HashiCorp employee
to N, N-1, and N-2, using the
backport/ent/x.x.x+ent
labels. If this PR is in the CE repo, you should only backport to N, using thebackport/x.x.x
label, not the enterprise labels.of a public function, even if that change is in a CE file, double check that
applying the patch for this PR to the ENT repo and running tests doesn't
break any tests. Sometimes ENT only tests rely on public functions in CE
files.
in the PR description, commit message, or branch name.
description. Also, make sure the changelog is in this PR, not in your ENT PR.