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.

Sign up for GitHub

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

Jump to bottom

typescript simple emitter #27596

Closed
wants to merge 1 commit into from
Closed

typescript simple emitter #27596

wants to merge 1 commit into from

Conversation

stacey-gammon
Copy link
Contributor

No description provided.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@stacey-gammon stacey-gammon force-pushed the 2019-19-12-simple-emitter-ts branch from 0d4282a to a625af2 Compare December 23, 2018 14:52
@elasticmachine
Copy link
Contributor

💔 Build Failed

@stacey-gammon stacey-gammon mentioned this pull request Jan 14, 2019
Closed
@stacey-gammon stacey-gammon force-pushed the 2019-19-12-simple-emitter-ts branch from a625af2 to 7887e21 Compare January 15, 2019 20:56
@stacey-gammon stacey-gammon mentioned this pull request Jan 15, 2019
Closed
@elasticmachine
Copy link
Contributor

💔 Build Failed

@stacey-gammon stacey-gammon force-pushed the 2019-19-12-simple-emitter-ts branch from 7887e21 to c86dc3b Compare January 15, 2019 21:40
@elasticmachine
Copy link
Contributor

💔 Build Failed

@stacey-gammon stacey-gammon force-pushed the 2019-19-12-simple-emitter-ts branch from c86dc3b to da161e8 Compare January 16, 2019 14:22
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@streamich
Copy link
Contributor

@lukeelmers this PR TypeScriptifies simple_emitter.js—if our plan is to move simple_emitter.js to the NP we can use this, otherwise let's close it.

@lukeelmers
Copy link
Member

Thanks!

Simple emitter is currently used in:

  1. ui/persisted_state
  2. ui/state_management
  3. vis_types_vislib

Both 1 and 2 are expected to be left behind by app arch over the course of migration (@ppisljar can confirm 1 in case I am mistaken), so that leaves vislib as the only place that will be using simple_emitter. I see that it is already on the list in #50670, so @flash1293 and team are probably the best ones to determine whether to repurpose this PR.

@flash1293
Copy link
Contributor

I discussed persisted_state offline with @ppisljar a while ago and my last stand was that it should get moved into visualizations short term and removed mid/long term - not sure about the details.

ui/state_management is planned to get left behind - we are currently migrating away from it and are scheduled to be done before NP cutover.

vis_types_vislib will continue to require it mid term, but as we are currently working on a replacement for the entire thing, I don't see much value in improving it. Inlining it into the plugin for the sunsetting period seems entirely fine to me especially since this is a relatively simple and stable piece of code.

My suggestion: Let's close this PR and inline simple emitter into both visualizations and vis_types_vislib to remove it mid-term after the NP cutover.

@lukeelmers
Copy link
Member

SGTM, in that case I'll close for now

@lukeelmers lukeelmers closed this Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@lukeelmers lukeelmers

Labels
Feature:NP Migration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@stacey-gammon @elasticmachine @streamich @lukeelmers @flash1293