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

Timefilter - replace SimpleEmitter with observables #43748

Merged
merged 28 commits into from
Aug 27, 2019

Conversation

lizozom
Copy link
Contributor

@lizozom lizozom commented Aug 22, 2019

Summary

Part of [NP Migration Phase I/II]: de-angularize & shim timefilter

Replace SimpleEmitter with observables.

FYI checked each event bieng triggered in this PR one by one

DevDocs

Timefilter exposes 5 events:

  • getEnabledUpdated$ - Fired when isTimeRangeSelectorEnabled \ isAutoRefreshSelectorEnabled are toggled.
  • getTimeUpdate$ - Fired when a user changes the timerange.
  • getRefreshIntervalUpdate$ - Fired when a user changes the the autorefresh settings.
  • getAutoRefreshFetch$ - Used when search_poll triggers an auto refresh.
  • getFetch$- Used when data is set, or the first time auto-refresh is enabled.

Old subscription:

timefilter.on('fetch', ...);
timefilter.off('fetch', ...);

New subscription:

const fetchSub = timefilter.getFetch$().subscrible(...);
fetchSub.unsubscribe();

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@lizozom lizozom requested review from a team as code owners August 22, 2019 10:23
@lizozom lizozom self-assigned this Aug 22, 2019
@lizozom lizozom added Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:AppArch labels Aug 22, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@lizozom
Copy link
Contributor Author

lizozom commented Aug 22, 2019

@chrisronline I fixed the error you saw when stopping auto refresh.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@clintandrewhall clintandrewhall left a comment

Choose a reason for hiding this comment

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

LGTM, and the changes wrt to Canvas seem very straightforward. Accepting for Canvas.

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

Looks good overall, just added a question about a line that could possibly removed.

@walterra
Copy link
Contributor

walterra commented Aug 26, 2019

@lizozom, I added @jgowdyelastic as a reviewer too since he's more into the new anomaly job wizards code.

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

Latest changes LGTM

@lizozom
Copy link
Contributor Author

lizozom commented Aug 27, 2019

Thanks. @jgowdyelastic let me know if you have any questions.

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

LGTM

@lizozom lizozom added release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. and removed release_note:skip Skip the PR/issue when compiling release notes labels Aug 27, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@lizozom lizozom merged commit 8cdd469 into elastic:master Aug 27, 2019
lizozom pushed a commit to lizozom/kibana that referenced this pull request Aug 27, 2019
* Replaced 'timeUpdate' and 'enabledUpdated' timefilter events with observables.

* Change enabledUpdated$ to a BehaviorSubject

* refreshIntervalUpdate + fixes in monitoring

* autoRefreshFetch

* getFetch + delete listenAndDigestAsync

* Removed SimpleEmitter parent

* Updated timefilter tests

* Post merge code updates in ML + type fixes

* visual editor unsubscribe

* removed unused import

* timefilter mock

* Import only from top level of timefilter

* Fixed typo in discover

* unsubscribe in monitoring

* Deleted two  tests relying on timefilter implementing EventEmitter

* Renamed subscribtion var name

* import path for fixing jest test ?

* Removed unused row
lizozom pushed a commit that referenced this pull request Aug 28, 2019
* Replaced 'timeUpdate' and 'enabledUpdated' timefilter events with observables.

* Change enabledUpdated$ to a BehaviorSubject

* refreshIntervalUpdate + fixes in monitoring

* autoRefreshFetch

* getFetch + delete listenAndDigestAsync

* Removed SimpleEmitter parent

* Updated timefilter tests

* Post merge code updates in ML + type fixes

* visual editor unsubscribe

* removed unused import

* timefilter mock

* Import only from top level of timefilter

* Fixed typo in discover

* unsubscribe in monitoring

* Deleted two  tests relying on timefilter implementing EventEmitter

* Renamed subscribtion var name

* import path for fixing jest test ?

* Removed unused row
@lizozom lizozom deleted the newplatform/timefilter/observables branch November 14, 2019 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:NP Migration release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. review v7.4.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants