-
Notifications
You must be signed in to change notification settings - Fork 8.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
[ML] Move nested property utilities and url state to packages #147912
Conversation
25f386c
to
e653e12
Compare
Pinging @elastic/ml-ui (:ml) |
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.
Code LGTM
I remerged |
@@ -239,7 +232,7 @@ export class PageUrlStateService<T> { | |||
* Hook for managing the URL state of the page. | |||
*/ | |||
export const usePageUrlState = <PageUrlState extends object>( | |||
pageKey: AppStateKey, | |||
pageKey: string, |
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 reckon it's important to validate the page id here. We could also infer PageUrlState
from the page key. I guess it's fine to merge AppStateKey
and the appropriate URL states from all plugins. WDYT?
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'd like to avoid having too plugin specific code in the packages which are used by multiple plugins. Instead I now did it the other way around and changed the required generic for usePageUrlState
to be a combo of the pageKey
definition and the original url state interface in 2f84808. Hope that's good enough for this iteration.
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Any counts in public APIs
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
ESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @walterra |
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.
LGTM! I reckon for further enhancement we can get rid of the generic type in usePageUrlState
, validate a pageKey
and infer the state from it. It'll require storing types inside of the package, but I assume it's fine as they don't affect the bundle size.
Summary
Part of #136182.
Effort to deduplicate code. Move nested property utilities and url state to packages.
Boilerplate for the packages was created likes this:
I consolidated the different
url_state.ts
files. One thing to note: Each one had its own definition forpageKey: AppStateKey
. I changed that and made it justpageKey: string
, I suspect it's good enough. Otherwise we'd have a reverse dependency on all consuming code. Alternative: We could refactor to require overriding a generic to pass in allowed values.Checklist