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

[rxjs] Establish interop of rxjs-6 and kbn-observable #21722

Merged
merged 3 commits into from
Aug 13, 2018
Merged

[rxjs] Establish interop of rxjs-6 and kbn-observable #21722

merged 3 commits into from
Aug 13, 2018

Conversation

rhoboat
Copy link

@rhoboat rhoboat commented Aug 7, 2018

Change the underlying symbol of kbn-observable to be what rxjs-6 uses.

Blocks #20272 (WIP)

Tests only pass with the changes made here. We could write more if you think necessary for confidence. Just kept it as simple as possible.

Question: this and this needs a change too, but not sure.

@rhoboat rhoboat added review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform enhancement New value added to drive a business result labels Aug 7, 2018
@rhoboat rhoboat self-assigned this Aug 7, 2018
@rhoboat rhoboat requested review from epixa, spalger and azasypkin August 7, 2018 12:57
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@@ -1,4 +1,4 @@
import symbolObservable from 'symbol-observable';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can symbol-observable be removed from the dependencies, then?

@@ -1,4 +1,4 @@
import symbolObservable from 'symbol-observable';
import { observable as Symbol_observable } from 'rxjs/internal/symbol/observable';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change the variable name away from camelCase? We don't generally use variables with underscores in them.

This applies to the other file as well.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, true. I got carried away using the same naming as rxjs.

@rhoboat
Copy link
Author

rhoboat commented Aug 10, 2018

I regenerated lock files because removing symbol-observable with yarn remove, and reinstalling with yarn kbn clean/bootstrap or yarn install didn't update the lockfiles. Regen made it update a lot of packages.

@rhoboat
Copy link
Author

rhoboat commented Aug 10, 2018

@epixa Ready for another review after CI finishes. I might have to revert lock file changes.

@rhoboat
Copy link
Author

rhoboat commented Aug 10, 2018

Yeah, I'm going to revert the lockfile changes. Something else is causing symbol-observable to remain, it's probably valid.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@rhoboat rhoboat mentioned this pull request Aug 10, 2018
7 tasks
@azasypkin
Copy link
Member

Since we finally came up with the decision to abandon kbn/observable in favor of RxJS 6+, let's just get rid of it in #17034 :)

@azasypkin azasypkin removed their request for review August 13, 2018 07:32
Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

LGTM with a green CI. Just had a brief conversation with @archanid and about this change being a blocker for other WIP PRs. It seems there is no harm in this interim solution, even though we'll get rid of @kbn/observable entirely in #17034 very soon.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@rhoboat rhoboat merged commit 4ebd3d3 into elastic:master Aug 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Feature:New Platform review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants