Skip to content

Timefilter - replace SimpleEmitter with observables#43748

Merged
lizozom merged 28 commits intoelastic:masterfrom
lizozom:newplatform/timefilter/observables
Aug 27, 2019
Merged

Timefilter - replace SimpleEmitter with observables#43748
lizozom merged 28 commits intoelastic:masterfrom
lizozom:newplatform/timefilter/observables

Conversation

@lizozom
Copy link
Copy Markdown
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
Copy Markdown
Contributor

Pinging @elastic/kibana-app-arch

@lizozom
Copy link
Copy Markdown
Contributor Author

lizozom commented Aug 22, 2019

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

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

Copy link
Copy Markdown
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
Copy Markdown
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 walterra requested a review from jgowdyelastic August 26, 2019 16:27
@walterra
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

lizozom commented Aug 27, 2019

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

Copy link
Copy Markdown
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
Copy Markdown
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