Move Storage ⇒ NP #49448
Move Storage ⇒ NP #49448
Conversation
…erenced to as "store" to avoid confusion.
lizozom
added
release_note:plugin_api_changes
|
Pinging @elastic/kibana-app-arch (Team:AppArch) |
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
💚 Build Succeeded |
| @@ -4,7 +4,7 @@ | |||
| * you may not use this file except in compliance with the Elastic License. | |||
| */ | |||
|
|
|||
| import { Storage } from 'ui/storage'; | |||
| import { Storage } from '../../../../../../src/plugins/kibana_utils/public'; | |||
There was a problem hiding this comment.
I don't think you have to have a path like this... I seem to recall Kibana paths aliased in Webpack so you can just do src/plugins/kibana_utils/public...?
There was a problem hiding this comment.
Asked and answered: #40446
Approving for Canvas... thanks!
There was a problem hiding this comment.
@clintandrewhall I'm familiar with that issue, but as far as I know it works for TS types but not for classes \ functions. Are you aware of any changes there?
There was a problem hiding this comment.
code LGTM
this is something that should be owned by platform team ideally @joshdover
|
@ppisljar we've got #41982 in the backlog regarding storages. However the PR implementation still provides a global-exposed constructor for |
|
@pgayvallet Could we still go ahead with that PR, as it cleared up a lot of unclear code, and migrate to your service once available? |
|
Yes of course, it's still an improvement compared to before. It's just that platform should not gain ownership of the feature until #41982 is done. Will properly review today |
|
|
||
| public setup(core: CoreSetup, { __LEGACY }: DataPluginSetupDependencies): DataSetup { |
There was a problem hiding this comment.
I'm a little scared about the changes on the way the storage is now accessed. What I mean is that before, the storage access was provided by a setup dependency (DataPluginSetupDependencies), and now the storage can be accessed basically from anywhere in the code base with a simple new Storage(window.localStorage), which may makes the migration more difficult when we'll implement #41982, as more calls may be added in places where there is no proper access to core APIs.
@joshdover WDYT ?
There was a problem hiding this comment.
@pgayvallet I agree that the way that storage was and is handled right now is not great. Even before this PR, apps would randomly create their own storage representation and pass it around. And this PR doesn't change the way this is handled (checkout x-pack/legacy/plugins/graph/public/app.js or x-pack/legacy/plugins/lens/public/app_plugin/plugin.tsx for example).
The __LEGACY wrapper was there only to mark the fact that Storage was imported from ui/public and not from NP. Now that it's moved to NP, we don't need that any more.
What we should do is, once platform team offers a core storage access service, is replace every occurrence of new Storage with that service.
💚 Build Succeeded |
* Move storage to kibana_utils * Updated all references to Storage and replace places where it was referenced to as "store" to avoid confusion. * fixed tests * Delete data legacy dependencies plugin * Imports fix * update snapshots
💔 Build Failed |
Summary
Resolves #47160
Storageis a wrapper around the browser's localStorage or sessionStorage, adding JSON handling for stored items.It now resides in NP under
kibana_utils.Dev Docs
Move
Storagetokibana_utils.Storageclass to NP, and introduce the interfaceIStorageWrapperfor when we only pass storage around.storagewas calledstoreChecklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers