-
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: convert mount backend code to TS #27349
Conversation
|
||
interface Args { | ||
mountModel: MountModel; | ||
mountType: 'secret' | 'auth'; |
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.
got these options from here:
* @param {string} [mountType=auth] - mount type can be `auth` or `secret` |
export default class MountBackendForm extends Component { | ||
@service store; | ||
@service flashMessages; | ||
type MountModel = MountSecretBackendModel | AuthEnableModel; |
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.
looks like this component is only rendered on 2 routes:
in both instances, the component directly receives the result of the model hook as its argument for @mountModel
. this is why i set up the types this way.
initially, i thought about setting the type to be either SecretEngineModel
or AuthMethodModel
. however, this setup provides more flexibility -- it allows for potential changes in the route model that may include properties not specifically part of the underlying Ember data models.
@@ -58,8 +72,8 @@ export default class MountBackendForm extends Component { | |||
} | |||
} | |||
|
|||
typeChangeSideEffect(type) { | |||
if (!this.args.mountType === 'secret') return; |
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.
TS caught this, which seemed like a typo so i fixed it 😮
@@ -6,8 +6,13 @@ | |||
import Route from '@ember/routing/route'; | |||
import { service } from '@ember/service'; | |||
|
|||
import type { ModelFrom } from 'vault/vault/route'; | |||
import type Store from '@ember-data/store'; |
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 noticed we've been importing the Store
type 2 different ways. one is this way, and the other is like this:
import type StoreService from 'vault/services/store'; |
however importing the type from @ember-data
works better since it recognizes that properties like isDestroying
etc are indeed on that type. the alternative way of importing Store
caused a bunch of type errors.
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.
That's strange, because in the store.d.ts
file we're extending Store from @ember-data/store
🤔 Should be harmless here since we're not using any of the extended methods here, but smells like something wrong in our ts setup
Build Results: |
CI Results: |
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.
✨ love these kinds of PRs! Thanks for doing this 🧹
🛠️ Description
Migrating a few files to TS, as having types will help us build the WIF UI.