-
Notifications
You must be signed in to change notification settings - Fork 33
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
Broken interoperability between Rxjs and symbol-observable #38
Comments
RxJS is now designed to have a polyfill loaded first, as polyfills should be. |
@benlesh ok, and in this scenario (when libraries don't polyfill) this ponyfill should also avoid to alter the global scope. |
Users of symbol-observable always get the same symbol, even if there will be multiple instances of the library, thus they depend on symbol-observable only. Your implementation doesn't normalize Symbol.observable, thus you depend on globals, based on the hypothetical future standard and order of importing polyfills. |
You should include all polyfills first, before any libraries. If you do that, RxJS will pick up the just-polyfilled And this package should be renamed to |
Just got hit by this. We're using redux (via read-dnd), Rxjs and mobx-utils. Rxjs and mobx-utils use a ponyfill. Redux uses this polyfill. The behaviour in our build was:
Rxjs and mobx-util observables are no longer compatible with each other. This behaviour is expected for a polyfill, as other comments suggest polyfills must be imported before all other modules. However this module very clearly advertises itself as a So I suggest either:
Rxjs ponyfill: https://github.com/ReactiveX/rxjs/blob/master/src/internal/symbol/observable.ts#L13 |
How about ? import { computed } from 'mobx'
import { Observable } from 'rxjs/Observable'
import { Observer } from 'rxjs/Observer'
export const toStream = <T>(expression: () => T): Observable<T> =>
Observable.create((observer: Observer<T>) => {
const computedValue = computed(expression)
return computedValue.observe(change => observer.next(change.newValue))
}) |
I got bitten by this issue too in a setup similar to @WearyMonkey's (redux + rxjs). In my case I got the error only in tests using the TestScheduler (from 'rxjs/testing') and only when Adding |
You're advising random modules to use this module, meaning that random modules will polyfill the environment for me. No thanks. That's kinda mean.
|
Sure, interop and agreements are developed only to let you break them. Obviously, there isn't enough inconsistency in the web, let's create brand new polyfill per project basis |
If that's all this module did, it would be great, and I would use it. But, this module is a polyfill, it sets |
In this issue, only rxjs breaks things, because bad interop is still better than a perfect disorder |
This has been resolved for some time. I tried again to see if I could replicate it by importing from https://codesandbox.io/s/vigilant-shirley-ve266?file=/src/index.ts |
I believe this is still an issue: Import E.g. https://codesandbox.io/s/lucid-wind-csjbo?file=/src/index.ts . Passing an observable built with the issue only shows up when:
|
Hi.
Due to latest decision to avoid polyfilling Symbol.observable and 6.0 release going public, there is a difference between how symbol observable is being defined in Rxjs and this repo.
Basically, there are 3 cases:
Symbol
at all)Symbol
is presented, butSymbol.observable
is not)Symbol.observable
in global scope).The problem is that for case #2 implementation differs:
@@observable
: https://github.com/ReactiveX/rxjs/blob/8c5d680494a8bc986e638f6138447917c7ba180f/src/internal/symbol/observable.ts#L13symbol-observable/es/ponyfill.js
Lines 9 to 10 in c849a83
Why is it an issue:
3rd party libraries relying on
symbol-observable
package to retrieveObservable
will fail to get one from RxJs.Possible solution
One solution I might think of is to avoid altering the global scope and simply fallback to
@@observable
(like RxJs does). However, it's a big breaking change for this package.The text was updated successfully, but these errors were encountered: