Skip to content
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 local storage utilities to package. #148049

Merged
merged 32 commits into from
Jan 5, 2023

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Dec 23, 2022

Summary

Part of #136182.
Follow up to #147912.

Moves multiple copies of useStorage() and related code to a package as a single source. The different copies with hard coded types have been adapted so useStorage() is now based on generics. Also moves duplicates of isDefined() to its own package.

Some duplicate settings related to FROZEN_TIER_PREFERENCE remain, this will be deduplicated in the follow up in #148063.

@walterra walterra added :ml backport:skip This commit does not require backporting v8.7.0 release_note:skip Skip the PR/issue when compiling release notes labels Dec 27, 2022
@walterra walterra marked this pull request as ready for review December 27, 2022 15:15
@walterra walterra requested a review from a team as a code owner December 27, 2022 15:15
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@walterra walterra requested review from alvarezmelissa87 and removed request for qn895 December 28, 2022 15:21
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to add comments to the public APIs before merging this, but otherwise code LGTM.

@@ -5,4 +5,4 @@
* 2.0.
*/

export { MlStorageContextProvider, useStorage } from './storage_context';
export { isDefined } from './src/is_defined';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment to this exported function to keep the 'public APIs without comments' count at 0?

storageKeys: readonly K[];
}

export function StorageContextProvider<K extends TStorageKey, T extends TStorage>({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment to this to keep the 'public APIs without comments' count at 0 for this package.

removeValue: <K extends TStorageKey>(key: K) => void;
}

export function isStorageKey<T>(key: unknown, storageKeys: readonly T[]): key is T {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment to this to keep the 'public APIs without comments' count at 0 for this package.

return storageKeys.includes(key as T);
}

export const MlStorageContext = React.createContext<StorageAPI>({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment to this to keep the 'public APIs without comments' count at 0 for this package.

Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gave this a test and looks to be working as expected. LGTM ⚡

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
aiops 455 459 +4
dataVisualizer 300 303 +3
ml 1594 1595 +1
transform 272 274 +2
total +10

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/ml-is-defined - 1 +1
@kbn/ml-local-storage - 3 +3
total +4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
aiops 737.4KB 739.1KB +1.7KB
dataVisualizer 377.1KB 378.6KB +1.5KB
ml 3.4MB 3.4MB +52.0B
transform 347.9KB 348.0KB +42.0B
total +3.3KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
aiops 5.2KB 5.3KB +141.0B
dataVisualizer 25.3KB 25.4KB +141.0B
total +282.0B
Unknown metric groups

API count

id before after diff
@kbn/ml-is-defined - 2 +2
@kbn/ml-local-storage - 5 +5
total +7

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @walterra

@walterra walterra merged commit dc1ae9e into elastic:main Jan 5, 2023
@walterra walterra deleted the ml-package-storage branch January 5, 2023 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting :ml release_note:skip Skip the PR/issue when compiling release notes v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants