Enforce shimmed plugin boundaries #52633
Conversation
|
@flash1293 thx, i will have a look and take care of |
|
Jenkins, test this. |
flash1293
added
the
release_note:skip
There was a problem hiding this comment.
Most of these SASS imports would be more manageable with moves like this if each folder has its own _index.scss manifest file. It still compiles as-is, but with the addition of the np_ready folders, they really should have their own manifests.
I can help with this structure if need be.
| @import 'components/field_chooser/index'; | ||
| @import 'angular/directives/index'; | ||
| @import 'angular/doc_table/index'; | ||
| @import 'np_ready/components/fetch_error/index'; |
There was a problem hiding this comment.
The np_ready folder should really have its own _index.scss manifest file.
| @import 'np_ready/components/fetch_error/index'; | ||
| @import 'np_ready/components/field_chooser/index'; | ||
| @import 'np_ready/angular/directives/index'; | ||
| @import 'np_ready/angular/doc_table/index'; | ||
|
|
||
| @import 'hacks'; | ||
|
|
||
| @import 'discover'; |
There was a problem hiding this comment.
These files and _mixins.scss should also be moved to np_ready folder
There was a problem hiding this comment.
Essentially this whole file should be moved to np_ready and then this file just imports the index file from there.
There was a problem hiding this comment.
That's a good point @cchaos , thanks for raising. I totally agree to have index files for the sub folders. But I would like to ignore the np_ready folder while doing this because it is just a temporary measure and will go away when we move discover to the new platform - so _index.scss for components and angular, but not for np_ready.
Also strictly speaking scss is not "np ready" because there is no way to compile scss in the new platform yet: #42185 It shouldn't be an issue for discover because once we move it over there probably will be a solution, but when we are cleaning up those imports it would be good to keep the future structure in mind (which is like today, but np_ready merged into the top level directly)
There was a problem hiding this comment.
The main reason to consider it for np_ready is if you have the manifest in plugin/np_ready/_index.scss like:
@import 'discover';
@import 'components/index';
...etc...
Then when you move/rename the whole np_ready folder, you don't have to change any of the imports in the _index.scss manifest file, you just change the single import of the index file.
Whereas right now you have many import paths directing to /np_ready/... explicitly that would need to change.
There was a problem hiding this comment.
You are right, I moved them into np_ready - doing it like this means the smallest change later when we migrate.
flash1293
added
the
Team:Visualizations
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
src/legacy/core_plugins/kibana/public/discover/np_ready/register_feature.ts
Show resolved
Hide resolved
|
Jenkins, test this. I can't reproduce this failing test locally, checking whether it's just flaky |
| @@ -0,0 +1,2 @@ | |||
| @import 'fetch_error/index'; | |||
| @import 'field_chooser/index'; | |||
There was a problem hiding this comment.
Missing the doc_viewer/index import
There was a problem hiding this comment.
Good catch, fixed!
| @import 'np_ready/editor/index'; | ||
| @import 'np_ready/listing/index'; | ||
| @import 'np_ready/wizard/index'; |
There was a problem hiding this comment.
Another opportunity to not list np_ready multiple times
There was a problem hiding this comment.
Thanks, took same approach as in discover
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
This PR introduces new linting rules for the shimmed plugins of the kibana app team (
'visualize,discover,dashboard,devTools,home) that validate no cross-imports are made and all imports from legacy world are centralized. Following the convention of other plugins everything within thenp_readydirectory within a plugin should be free of these kind of imports and nothing from within these directories is imported directly somewhere elsePieces that are not covered by the shim have not been moved into
np_ready. As the visulize embeddable is going to move into the visualizations plugin, it is moved out of the visualize plugin completely into its own directoryvisualize_embeddable.