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 use of instanceof breaks subscription with multiple bundles #3828

Closed
Avocher opened this issue Jun 11, 2018 · 4 comments
Closed

Rxjs use of instanceof breaks subscription with multiple bundles #3828

Avocher opened this issue Jun 11, 2018 · 4 comments

Comments

@Avocher
Copy link

Avocher commented Jun 11, 2018

Bug Report

Current Behavior
Hi, we are building a components that exposes rxjs observables for plugins to connect to. Our component is primarely used prebundled, which means that our plugin creators might have double of some parts of rxjs. Though, somewhat wastefull this worked fine with rxjs 5, but after upgrading to rxjs 6, things started breaking in Firefox and Chrome Canary 69. Here's the error message they get when trying to subscribe:

TypeError: You provided an invalid object where a stream was expected. You can provide an Observable, Promise, Array, or Iterable.

Which pointed at the subscribeTo function, where I suspect that this line is the culprit:

if (result instanceof Observable) {

instanceof seems to be a bit unreliable and appears to work differently under the hood in each browser, especially if the class appears twice and originated from the same source.

Reproduction

  1. Create two bundles referencing rxjs 6.2.0, one exposing an observable, another subscribing to it and using its own operators.
  2. Try to subscribe.

Expected behavior
Should subscribe since there is nothing (?) stopping it from doing so.

Environment

  • Chrome 69 & Firefox 60
  • RxJS version: 6.2.0

If anyone knows a workaround I'd be happy with that, since I know some would probably regard this as a non-issue.

@benlesh
Copy link
Member

benlesh commented Jun 11, 2018

It sounds like you're bundling two instances of RxJS. That's not going to be good for your app size. Your libraries shoudl have RxJS as a peer dependency, and that will solve the problem most likely.

However, there are workarounds:

  1. Load a polyfill for Symbol.observable. RxJS 5 used to polyfill it for you. RxJS 6 doesn't, because libraries shouldn't be doing that. If Symbol.observable is present, then each copy of RxJS will coerce to it's own Observable type using that.

  2. Use a from call to wrap observables you think come from other libraries.

... but the real solution here is that you'd want RxJS to be a peer dependency.

@benlesh benlesh added type: question AGENDA ITEM Flagged for discussion at core team meetings labels Jun 11, 2018
@benlesh
Copy link
Member

benlesh commented Jun 11, 2018

We might start using our isObservable implementation internally, and that could solve this issue, but it's also sort of a smell in the usage here. We'll discuss it at the next core team meeting.

@Avocher
Copy link
Author

Avocher commented Jun 14, 2018

Thank you for the help Ben! Yes, our current setup isn't ideal. Your suggestions will have to serve as a band aid for the time being until we get our distribution setup sorted out.

@benlesh benlesh removed the AGENDA ITEM Flagged for discussion at core team meetings label Aug 1, 2018
@benlesh
Copy link
Member

benlesh commented Aug 1, 2018

After some discussion with the team today, we've decided to leave the instanceof checks as is. Because:

  1. There are performance implications of moving away from instanceof checks
  2. The work around is as easy as polyfilling Symbol.observable.

@benlesh benlesh closed this as completed Aug 1, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants