-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: Move useOpenApi and getHelpUrl methods to util #27764
Conversation
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.
Great cleanup work to move us towards more ember-data happy models! Just a couple non-blocking comments
}; | ||
|
||
export function getHelpUrlForModel(modelType: string, backend: string) { | ||
const urlFn = OPENAPI_POWERED_MODELS[modelType as keyof typeof OPENAPI_POWERED_MODELS] as ( |
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.
do we need both keyof and typeof here?
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.
Yup, because OPENAPI_POWERED_MODELS
is a const not an interface, so we need typeof so typescript is happy
const model = this.store.createRecord(modelName, {}); | ||
// Generated items don't have this method -- helpUrl is calculated in path-help.js line 101 | ||
const helpUrl = model.getHelpUrl(this.mount); | ||
// Generated items don't have helpUrl method -- helpUrl is calculated in path-help.js line 101 |
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.
thank you for this comment! do we have test coverage that assures removing the || newModel.proto().getHelpUrl(backend);
of the conditional in path-help is alright?
From what I gather it seems like all non-generated items have paths defined in the new helper, getHelpUrlForModel
and any generated items should otherwise follow the pattern of /v1/${apiPath}${path.slice(1)}?help=true
- just wanted to double check!
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.
Good question! I removed the ||
because the first statement would never be falsey.
// original
helpUrl = `/v1/${apiPath}${path.slice(1)}?help=true` || newModel.proto().getHelpUrl(backend);
even if apiPath
and path.slice(1)
resolve to empty strings, the first part will resolve at minimum to /v1/?help=true
.
Otherwise your comment is true -- if we have the base model for it, the helpUrl should be defined in getHelpUrlForModel
. If it's generated (like for userpass users) then it follows the pattern in line 101 of path-help 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.
Also, this is a skipped test -- I just didn't want any instances of getHelpUrl
laying around so I could be sure I got them all 😂
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.
great thinking! nice to keep up with tech debt while we have context. awesome! that all makes sense, thanks for explaining.
* Add map between model types and helpUrls, update tests * replace modelProto.getHelpUrl with new helper util * Remove all useOpenApi and getHelpUrl instances from models * Add missing auth config model type
* Add map between model types and helpUrls, update tests * replace modelProto.getHelpUrl with new helper util * Remove all useOpenApi and getHelpUrl instances from models * Add missing auth config model type
Description
This is a cleanup item to remove
useOpenApi
andgetHelpUrl
from the models (which don't have anything to do with the shape of the data) and into a util that thepath-help
service can then utilize to get the appropriate data based on the model type and backend path. There should be no user-facing changes with this PR, but is foundation work for eventually upgrading to Ember Data 5