Prep visualizations plugin for NP migration. #44839

lukeelmers · 2019-09-04T23:52:25Z

This is a follow-up PR which reverts some of the changes introduced in #44644 as a part of resolving #44306.

I added a comment with some feedback on the original PR, and also have been working on a draft PR that accomplishes some similar things.

Notes on my changes are below; let's discuss further in this PR if anyone has additional feedback so that we are all aligned on conventions we want to use.

This PR implements some feedback on #44644 by making the following changes:

Reverts changes to imports which were pulling shim contracts from the np_ready directory. I realize this is how embeddable_api currently implements that directory, but I would argue that we shouldn't expose usage of np_ready directory to consumers of the shim as it introduces confusion and is subject to change. I propose only exporting shims from a top level public/legacy, which is what we have already documented in MIGRATION.md anyway.
kibana.json manifest moved to the top level & added data plugin as dependency
public/np_ready/public felt like a redundant directory structure, so moved items to np_ready directly
Moved mocks up to public/mocks since they included bypasses for the eslint imports rule, which by definition means they aren't "np_ready"
Updated the names used in shim imports to be visualizationsSetup to be consistent with how we've named other shims. This is also consistent with the VisualizationsSetup public interface we export.
Removed visTypeAliasRegistry from the legacy setup dependencies, as this is already NP ready

This PR also replaces #43730 by making the following changes:

Updates structure of vis plugin for consistency with our shimming conventions
Ensures public contracts from ui/vis that need to live in the visualizations plugin are re-exported
Added some clarifying comments for future devs

Next steps / other considerations:

Should consider renaming / tweaking the interface for visTypeAliasRegistry as we do further work on this plugin. visualizations.types.visTypeAliasRegistry seems awkward. visualizations.types.registerVisualizationAlias might make more sense?
Consider updating embeddable_api as well to use the same np_ready structure. Not sure if this is worth the effort as we are quite close to completing migration on this.
Audit the legacy/NP migration table in MIGRATION.md to ensure it is consistent with the new implementation. That way people know where to find the shims they need.
Actually move the code and re-export legacy contracts from ui/vis and follow the rest of the steps in Visualizations 👉 New Platform #44121

elasticmachine · 2019-09-04T23:52:27Z

Pinging @elastic/kibana-app-arch

elasticmachine · 2019-09-05T00:21:47Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

mattkime · 2019-09-05T00:29:09Z

Reverts changes to imports which were pulling shim contracts from the np_ready directory. I realize this is how embeddable_api currently implements that directory, but I would argue that we shouldn't expose usage of np_ready directory to consumers of the shim as it introduces confusion and is subject to change. I propose only exporting shims from a top level public/legacy, which is what we have already documented in MIGRATION.md anyway.

Should we change embeddable_api? People often learn by example.

sainthkh · 2019-09-05T02:21:28Z

Thanks for the thorough review and improvements. Your code is better than mine. I agree with your changes.

When I was writing np_ready, I learned the convention from the current code base.

I learned importing setup from public/legacy from 2 modules: embeddable_apiand core_plugins/data.
I learned public/np_ready/public structure from embeddable_api and dashboard_embeddable_container.

And I agree with @mattkime. We learn by examples. Many future developers/contributors will read how current plugins are implemented and learn from them. Actually, that's what I did.

So, I think making these 3 modules follow the new rules (no import from legacy and no public/np_ready/public) will be good for new developers as a reference.

Actually, I thought about moving embeddable_api to src/plugins now. But it's impossible now because it's used in dashboard and discover, etc. We will need setup shim anyway.

And how about adding setup as a shim example to Bonus: Tips for complex migration scenarios section in MIGRATION.md?

ppisljar

code LGTM, however tests are currently broken

src/legacy/core_plugins/kibana/public/visualize/listing/visualize_listing.js

lukeelmers · 2019-09-05T23:22:03Z

Thanks for the thorough review and improvements. Your code is better than mine. I agree with your changes.

Thanks @sainthkh - but there's no "better" or "worse" here, just a question of how to make everything consistent. Your contribution got the conversation started, which is the most important thing ❤️

Actually, I thought about moving embeddable_api to src/plugins now. But it's impossible now because it's used in dashboard and discover, etc. We will need setup shim anyway.

@stacey-gammon or @ppisljar Do you think it is worth shuffling the directories around for the shims to be consistent in embeddables_api? Considering that we are hoping to move it fully to the new platform in the nearer term, I'm not sure if it is worth taking the time to make this change if the shim will soon be removed anyway?

And how about adding setup as a shim example to Bonus: Tips for complex migration scenarios section in MIGRATION.md?

This is a great idea, we could probably be more explicit here since we only briefly reference np_ready.

sainthkh · 2019-09-06T00:39:43Z

@lukeelmers If you think fixing embeddables_api is too much for now, it would be a good idea to write things like "It is recommended to follow the directory structure of visualizations in future plugins." in MIGRATION.md doc.

stacey-gammon · 2019-09-06T12:34:12Z

Actually, I thought about moving embeddable_api to src/plugins now. But it's impossible now because it's used in dashboard and discover, etc. We will need setup shim anyway.

It's not impossible actually, legacy plugins can use new platform plugins, so we can move forward with this... I think now actually. I know Vadim had it on his radar, but will be out for a few weeks. I'm pretty sure this was the last blocker (#43416) and it's merged (the only blockers are actually if the embeddable api is using legacy stuff - that can't happen, so all legacy dependencies need to get moved over first.). So if you are interested you should be able to tackle that and that would be awesome! Though... you should probably wait till #44707 is merged, otherwise there will be a ton of merge conflicts. You could also try working off it cherry-picked but no telling how much will change during review process.

…port paths.

…le anyway.

…out of legacy deps.

lukeelmers · 2019-09-10T15:10:35Z

retest

lukeelmers · 2019-09-10T16:11:26Z

Do you think it is worth shuffling the directories around for the shims to be consistent in embeddables_api? Considering that we are hoping to move it fully to the new platform in the nearer term, I'm not sure if it is worth taking the time to make this change if the shim will soon be removed anyway?

@stacey-gammon and I discussed and agreed that we don't need to prioritize updating the embeddables shims since we are looking to move to NP soon enough anyway. However, if whoever works on the embeddables > NP move finds it easier to update the shims as part of that process, they are of course free to do so.

elasticmachine · 2019-09-10T17:51:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…ation_behaviour * 'master' of github.com:elastic/kibana: (24 commits) [Console] SQL template with triple quote in completion (elastic#45248) [ML] Data Frames: Cards as links (elastic#45254) fix(code/frontend): should show updating instead of cloning when updating (elastic#45238) fix(code/frontend): fix document search result from (elastic#45236) disable another flaky suite (elastic#45323) (elastic#45330) disable flaky suite (elastic#45105) skip flaky suite (elastic#43069) skip flaky suite (elastic#45089) disable jest suite that has no enabled tests (elastic#44250) disable flaky test (elastic#45317) disable flaky test (elastic#45315) [DOCS] Creates developer folder (elastic#45280) [SIEM] Changes ML conditional links to use tabs, fixes a small bug with null filterQuery (elastic#45218) [skip-ci][Maps] Update search docs (elastic#45307) Revert "[skip ci][Maps] Update search document section with ne… (elastic#45301) Prep visualizations plugin for NP migration. (elastic#44839) Replace Discover chart with elastic-charts (elastic#43788) [skip ci][Maps] Update search document section with new features (elastic#44819) Revert "Revert "[ci] compress jobs for CI stability" (elastic#44584)" add src/plugins to the list of plugin dirs to watch (elastic#45033) ... # Conflicts: # src/legacy/core_plugins/console/public/src/utils.js # src/legacy/core_plugins/console/public/tests/src/utils_string_expanding.txt

lukeelmers added review Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v8.0.0 Team:AppArch release_note:skip Skip the PR/issue when compiling release notes v7.5.0 Feature:NP Migration labels Sep 4, 2019

lukeelmers requested a review from mattkime September 4, 2019 23:52

lukeelmers self-assigned this Sep 4, 2019

This was referenced Sep 4, 2019

Restructure vis plugin & export types contracts #43730

Closed

Visualizations Plugin -> New Platform Ready #44644

Merged

ppisljar approved these changes Sep 5, 2019

View reviewed changes

streamich mentioned this pull request Sep 5, 2019

Visualizations 👉 New Platform #44121

Closed

13 tasks

stacey-gammon reviewed Sep 5, 2019

View reviewed changes

src/legacy/core_plugins/kibana/public/visualize/listing/visualize_listing.js Show resolved Hide resolved

lukeelmers added 7 commits September 9, 2019 15:26

Move NP kibana.json manifest to root of plugin.

c2e6973

Add data plugin dependency to kibana.json NP manifest.

1878011

Allow vis contracts to be consumed without importing from np_ready.

e938304

Remove unnecessary nested public directory.

79911a0

Revert updates from elastic#44644 that changed visualizations shim im…

afd7f56

…port paths.

Move mocks outside of np_ready since they were ignoring the eslint ru…

ea37af9

…le anyway.

Merge with changes from elastic#43730.

e078a78

lukeelmers added 2 commits September 9, 2019 15:26

Remove unnecessary exported interfaces for vis types alias, and move …

f3bd083

…out of legacy deps.

Remove kibana.json manifest which breaks legacy plugins.

2d18abd

lukeelmers force-pushed the fix/vis-plugin branch from 6e30a16 to 2d18abd Compare September 9, 2019 21:26

Merge branch 'master' of github.com:elastic/kibana into fix/vis-plugin

c839f09

lukeelmers merged commit 6039709 into elastic:master Sep 10, 2019

lukeelmers deleted the fix/vis-plugin branch September 10, 2019 19:02

wylieconlon mentioned this pull request Sep 11, 2019

[lens] Draft against 7.x #45459

Closed

wylieconlon pushed a commit to wylieconlon/kibana that referenced this pull request Sep 11, 2019

Prep visualizations plugin for NP migration. (elastic#44839)

3362cea

wylieconlon mentioned this pull request Sep 11, 2019

[7.x] Prep visualizations plugin for NP migration. (#44839) #45474

Merged

wylieconlon pushed a commit that referenced this pull request Sep 12, 2019

Prep visualizations plugin for NP migration. (#44839) (#45474)

6ded9a9

lukeelmers mentioned this pull request Nov 12, 2019

App Architecture roadmap for New Platform migration #31968

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prep visualizations plugin for NP migration. #44839

Prep visualizations plugin for NP migration. #44839

lukeelmers commented Sep 4, 2019

elasticmachine commented Sep 4, 2019

elasticmachine commented Sep 5, 2019

mattkime commented Sep 5, 2019

sainthkh commented Sep 5, 2019

ppisljar left a comment •

edited

Loading

lukeelmers commented Sep 5, 2019 •

edited

Loading

sainthkh commented Sep 6, 2019

stacey-gammon commented Sep 6, 2019

lukeelmers commented Sep 10, 2019

lukeelmers commented Sep 10, 2019

elasticmachine commented Sep 10, 2019

Prep visualizations plugin for NP migration. #44839

Prep visualizations plugin for NP migration. #44839

Conversation

lukeelmers commented Sep 4, 2019

elasticmachine commented Sep 4, 2019

elasticmachine commented Sep 5, 2019

💔 Build Failed

mattkime commented Sep 5, 2019

sainthkh commented Sep 5, 2019

ppisljar left a comment • edited Loading

Choose a reason for hiding this comment

lukeelmers commented Sep 5, 2019 • edited Loading

sainthkh commented Sep 6, 2019

stacey-gammon commented Sep 6, 2019

lukeelmers commented Sep 10, 2019

lukeelmers commented Sep 10, 2019

elasticmachine commented Sep 10, 2019

💚 Build Succeeded

ppisljar left a comment •

edited

Loading

lukeelmers commented Sep 5, 2019 •

edited

Loading