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

Should we just detect that it fulfills the interface instead of using Symbol? #1

Closed
jamestalmage opened this issue Apr 25, 2016 · 19 comments

Comments

@jamestalmage
Copy link

jamestalmage commented Apr 25, 2016

Using Symbol is not 100% reliable unless every implementation installs the symbol the same way.

One recent issue: zenparsing/zen-observable#9

Should we detect similar to is-promise? Via feature detection:

return typeof obj.subscribe === 'function' && typeof obj.forEach === 'function';

Why is Symbol detection preferred?

@sindresorhus
Copy link
Owner

I don't understand. I use zen-observable for testing this module. Seems to work fine. https://github.com/sindresorhus/is-observable/blob/master/test.js

Symbol detection is better as it's guaranteed to work. subscribe and forEach are very common properties, so I can foresee a lot of false-positives.

The reason is-promise uses a naive check is that Promises are required to support thenables, meaning just objects with a .then() functions. This is probably because Symbols were not a thing when Promises were incarnated. We now do have Symbols.

@jamestalmage
Copy link
Author

I was definitely getting false negatives on zen-observables using observable-to-promise (it was throwing saying it wasn't an observable)

Not sure why. Might be related to recent changes to symbol-observable or something else. I just dropped observable-to-promise and called forEach to convert it.

It seems to be an issue with whatever version shipped with ava@0.14 (we've bumped since releasing). Also, I had loaded RxJS as well - maybe that had something to do with it

@sindresorhus
Copy link
Owner

Might be related to recent changes to symbol-observable or something else.

I ran the test with latest (0.2.3).

@sindresorhus
Copy link
Owner

// @Blesh

@benlesh
Copy link

benlesh commented Apr 25, 2016

Symbol detection is preferred because subscribe is a common method name in other APIs. Redux stores, for example, have a subscribe method.

However, the only way to be sure you have an observable is to find Symbol.observable and call it to get a guaranteed observable back. This is for exactly the same reason as above. An object may have a subscribe method on it, and Symbol.observable on it, and the subscribe has nothing to do with it being an observable... Redux store is again a good example of that.

@jamestalmage
Copy link
Author

Passes:

const isObservable = require('is-observable');
const ZenObservable = require('zen-observable');
const assert = require('assert');

assert(isObservable(new ZenObservable(() => {})), 'zen');

Fails:

const ZenObservable = require('zen-observable');
const isObservable = require('is-observable');
const assert = require('assert');

assert(isObservable(new ZenObservable(() => {})), 'zen');

Only difference is the import order of is-observable vs zen-observable

@SamVerschueren
Copy link

I experience the same false-negatives in listr when I try to use is-observable.

@andywer
Copy link

andywer commented Sep 16, 2016

Not sure how "hot" this topic still is, but how about:

Boolean(obj && typeof obj.subscribe === 'function' && obj.constructor.name === 'Observable')

It's still hacky, but at least it's plausible. Used in listr now and tested with rxjs (version 5.0.0-beta) and zen-observable.

@sindresorhus
Copy link
Owner

@andywer This is what various Observable libs returns with obj.constructor.name:

  • zen-observable => Observable
  • xstream => Stream
  • RxJS 5 => ArrayObservable
  • most => Stream

@andywer
Copy link

andywer commented Mar 14, 2017

@sindresorhus Ohh dear lord... 🙈 🙃

@benlesh
Copy link

benlesh commented Mar 15, 2017

RxJS 5 will return any number of things from that depending on what you're doing. ScalarObservable, Observable, PromiseObservable etc.

@SamVerschueren
Copy link

I cannot reproduce this issue anymore. This example for instance failed, like @jamestalmage described above.

const ZenObservable = require('zen-observable');
const isObservable = require('is-observable');
const assert = require('assert');

assert(isObservable(new ZenObservable(() => {})), 'zen');

Zen as well as RxJS both worked, independent on the order I imported them. Am I missing something or can we close this one?

@SamVerschueren
Copy link

Can someone confirm my findings? I really want to switch to is-observable instead of rolling my own implementation https://github.com/SamVerschueren/listr/blob/master/lib/utils.js#L4.

@andywer
Copy link

andywer commented Oct 24, 2017

Seems to work now 👍
(Tested with latest versions of is-observable & zen-observable)

@SamVerschueren
Copy link

Darn, this happens again with RxJS@6. I don't have any problems with RxJS@5 though.

const {Observable} = require('rxjs');
const isObservable = require('is-observable');
const assert = require('assert');

assert(isObservable(new Observable(() => {})), 'rxjs');

This works with RxJS@5 but not with RxJS@6

If I import is-observable first, it works with RxJS@6 as well.

@SamVerschueren SamVerschueren reopened this May 9, 2018
@SamVerschueren
Copy link

SamVerschueren commented May 9, 2018

It looks like it is unrelated to the opening post. Apparently, RxJS@6 doesn't polyfill the RxJS symbol anymore. This is what the RxJS symbol looks like internally

const symbol = typeof Symbol === 'function' && Symbol.observable || '@@observable';

This means that it looks if Symbol.observable exists and otherwise it falls back to @@observable.

So for Listr, I used the following implementation

const symbolObservable = typeof Symbol === 'function' && Symbol.observable || '@@observable';

const isObservable = value => Boolean(value && value[symbolObservable] && value === value[symbolObservable]()) || require('is-observable')(value);

And now the detection works perfect. Not sure how to go forward for is-observable? Maybe keep what we have right now but add an or check with the RxJS@6 way of doing it?

@benlesh
Copy link

benlesh commented May 24, 2018

It looks like it is unrelated to the opening post. Apparently, RxJS@6 doesn't polyfill the RxJS symbol anymore.

@SamVerschueren libraries shouldn't be polyfilling for you, devs need to add polyfills first, then load libraries. I had to learn that the hard way with RxJS. :)

PS: That little bit in RxJS with the @@observable is a bit of a hack, I know, but it's there to make sure two different versions of RxJS loaded via Node (it happens) will work together.

@SamVerschueren
Copy link

@benlesh I totally agree and understand that libraries shouldn't polyfill. The thing is that is-observable uses symbol-observable which does the polyfilling and just isn't clear from user point of view. IMO, symbol-observable shouldn't be polyfilling Symbol.observable.

const rxjs = require('rxjs');
const isObservable = require('is-observable');

So RxJS falls back to @@observable, but afterwards a dependency of is-observable polyfills Symbol.observable. See the issue created in symbol-observable benlesh/symbol-observable#38.

@sindresorhus
Copy link
Owner

sindresorhus commented May 28, 2019

This module no longer uses the Symbol.observable polyfill: https://github.com/sindresorhus/is-observable/releases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants