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

[Maps] Move redux reducers and store logic to NP #58294

Merged
merged 33 commits into from
Mar 12, 2020

Conversation

kindsun
Copy link
Contributor

@kindsun kindsun commented Feb 21, 2020

This PR accomplishes the following:

  • Create new NP Maps plugin
  • Move Maps redux store and reducers to NP and leverages them from the legacy client
  • Initialize some services on the NP side to support reducer file operations

A few things worth noting or focusing on:

  • Something like 80% of this really is just physical movement of the code and updated paths
  • This is part of a bigger blocker issue so I didn't place too much focus on TS updates. We can generate follow-up issues if needed though
  • Since we're "between two worlds" for the moment, a lot of these comments are necessary to avoid linting errors: // eslint-disable-next-line @kbn/eslint/no-restricted-paths. As we continue moving the architecture over, there will continue to be more, then less, then none.
  • There are now 2 kibana_services.js files, one in the legacy plugin, one in the NP plugin. Eventually these will be consolidated into one, but for now we're just initializing the services (really just one in NP for now) required to support the store/reducers
  • Action const declarations are temporarily redundant. They're in legacy and they're in NP and they're surrounded by comments along the lines of: // NP Migration // Temporarily redundant with x-pack/plugins/maps/public/actions/map_actions.js Since the both the reducers and the un-migrated action functions need these, the alternative is to move them and then import each of them all back to legacy, only to change it back when we're a few steps further into this migration. That just feels unnecessary but I'm open to pushback
  • Files in the common folder are temporarily redundant as well. It's just easier that sifting through every time to figure out what's moved vs. not. In subsequent PRs these will be checked for updates (which are pretty infrequent)
  • There were some hurdles with initializing embeddables (again we're in a weird spot until we get them fully moved over) but this was for the most part ironed out and broken out into a not-so-complicated solution for this PR. This PR builds on that a little but mostly just mentioning here for context.

@kindsun kindsun added [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 Feature:NP Migration v7.7.0 labels Feb 21, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

@kindsun kindsun force-pushed the np-port-reducers-and-related-logic branch from fe702cc to e127a90 Compare February 24, 2020 21:13
@kindsun kindsun marked this pull request as ready for review March 5, 2020 16:54
@kindsun kindsun requested a review from a team as a code owner March 5, 2020 16:54
@kindsun kindsun added the release_note:skip Skip the PR/issue when compiling release notes label Mar 5, 2020
@kindsun
Copy link
Contributor Author

kindsun commented Mar 5, 2020

Something's breaking tooltips in this PR. Looking into it Fixed!

@nreese
Copy link
Contributor

nreese commented Mar 6, 2020

This is part of a bigger blocker issue so I didn't place too much focus on TS updates. We can generate follow-up issues if needed though

+1. Migrating to NP is complex enough. No reason to add TS to the mix

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

This is a great start at migrating to NP.

One question about the common code migrated to NP. Looks like it was not removed from legacy platform. Would you mind putting comments around it like the action declarations so we know its duplicated?

@@ -4,12 +4,12 @@
* you may not use this file except in compliance with the Elastic License.
*/

// @ts-nocheck
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do? Does it disable ts linting for the file?

Copy link
Contributor Author

@kindsun kindsun Mar 6, 2020

Choose a reason for hiding this comment

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

Yes. We were doing quite a few @ts-ignores and this was just cleaner. Also I needed to combine @ts-ignore and @kbn/eslint/no-restricted-paths for line 12 and couldn't find a good way to do it. I'm sure it's possible but overall this just seemed cleaner and temporary

@@ -0,0 +1,160 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is .ts in legacy and should stay .ts in NP.

This copy is also really stale. For example, METRIC_TYPE has been renamed AGG_TYPE and lots of values have been converted to enums for TS changes in legacy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry yes, this was one of the first folders moved over. One thing about moving the folders over, it that you have to watch the old folders for changes. I kept reducers on my radar but let the common folder changes slip by. Thanks for catching

x-pack/plugins/maps/common/i18n_getters.js Outdated Show resolved Hide resolved
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

lgtm - I think this PR needs 2 reviews so lets wait for Thomas's review before merging
code review and tested in chrome

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

This is great, thx

Main comment imho is to remove all duplicated constant declaration. In legacy, re-import from np. This would apply to the main-constants, the action-type names, and a couple of utility functions. Thanks!

x-pack/legacy/plugins/maps/common/constants.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/maps/common/i18n_getters.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/maps/common/parse_xml_string.js Outdated Show resolved Hide resolved
x-pack/legacy/plugins/maps/common/parse_xml_string.test.js Outdated Show resolved Hide resolved
x-pack/legacy/plugins/maps/public/actions/map_actions.js Outdated Show resolved Hide resolved
x-pack/legacy/plugins/maps/public/plugin.ts Outdated Show resolved Hide resolved
x-pack/plugins/maps/kibana.json Show resolved Hide resolved
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

thx for removing redundancies and @ts-nocheck.

np is happening!

@kindsun kindsun merged commit 5cab334 into elastic:master Mar 12, 2020
kindsun pushed a commit to kindsun/kibana that referenced this pull request Mar 12, 2020
* Plugin file and services in place. Some redux logic ported

* Port and update index pattern util

* Move reducers over to NP. Update refs in legacy

* Port inspector to NP

* Move some kibana services init to NP. Some cleaning

* Clean up work not related to reducers/store

* Ignore temp imports from NP. Clean up of changes unrelated to this PR

* More cleanup. Check injected vars avab. before calling to handle dashboard case

* Bind embeddables services the same way Maps app services bound. Create function for eventual init in NP

* Call binding from constructor. Fix npStart plugins arg

* Adapt changes from master

* Register inspector views for embeddable. Add NP folder to i18n

* Clean up. Add comments. Move inspector map view registration to NP

* Remove unused inspector files in legacy

* Move full screen action to legacy

* Add in missing tooltip updates

* Review feedback. Update constants and i18n_getters to latest in NP

* Review feedback. Add redundancy comments to common files redundant in legacy and NP

* Remove unneeded copy of parse xml string test in legacy

* Review feedback. Remove redundant portions. Export from NP where possible. General clean up

* Remove remaining refernce and case for 'TOUCH_LAYER'. It's never used
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 12, 2020
* master: (45 commits)
  skip flaky suite (elastic#59717)
  UI Metrics use findAll to retrieve all Saved Objects (elastic#59891)
  [Discover] Migrate Context mocha tests to use Jest (elastic#59658)
  [Maps] Move redux reducers and store logic to NP (elastic#58294)
  rebalance x-pack groups (elastic#58930)
  [Discover] Reimplement $route.reload when index pattern changes (elastic#59877)
  [Upgrade Assistant Meta] Breaking changes issue template (elastic#59745)
  Skip CI based on changes in PR (elastic#59939)
  [ML] Transforms: Replace KqlFilterBar with QueryStringInput. (elastic#59723)
  [ML] Functional tests - stabilize date_nanos test (elastic#59986)
  [ML] Typescripting client side endpoint functions (elastic#59928)
  a11y tests on adding columns to discover table (elastic#59375)
  fix graph plugin config path (elastic#59540)
  fix vega config issues (elastic#59737)
  [Upgrade Assistant] Open And Close Slight Refactor (elastic#59890)
  [ML] Adding shared services to ml setup contract (elastic#59730)
  [Visualize] Fix linked search behavior (elastic#59690)
  [ML] Register NP ML plugin for Kibana management section. (elastic#59762)
  [Lens] Adds using queries/filters for field existence endpoint (elastic#59033)
  Delete FilterStateManager and QueryFilter :-D (elastic#59872)
  ...
kindsun pushed a commit that referenced this pull request Mar 12, 2020
* Plugin file and services in place. Some redux logic ported

* Port and update index pattern util

* Move reducers over to NP. Update refs in legacy

* Port inspector to NP

* Move some kibana services init to NP. Some cleaning

* Clean up work not related to reducers/store

* Ignore temp imports from NP. Clean up of changes unrelated to this PR

* More cleanup. Check injected vars avab. before calling to handle dashboard case

* Bind embeddables services the same way Maps app services bound. Create function for eventual init in NP

* Call binding from constructor. Fix npStart plugins arg

* Adapt changes from master

* Register inspector views for embeddable. Add NP folder to i18n

* Clean up. Add comments. Move inspector map view registration to NP

* Remove unused inspector files in legacy

* Move full screen action to legacy

* Add in missing tooltip updates

* Review feedback. Update constants and i18n_getters to latest in NP

* Review feedback. Add redundancy comments to common files redundant in legacy and NP

* Remove unneeded copy of parse xml string test in legacy

* Review feedback. Remove redundant portions. Export from NP where possible. General clean up

* Remove remaining refernce and case for 'TOUCH_LAYER'. It's never used
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants