tc39 / proposal-observable

Observables for ECMAScript
https://tc39.github.io/proposal-observable/
3.06k stars 90 forks source link

Should Observable.from use a brand check to confirm argument is Observable #169

Open jhusain opened 7 years ago

jhusain commented 7 years ago

Observable.from currently only checks that the constructor property of the input is equal to the the this object of Observable.from. This seems easy to spoof. It's straightforward to create an object with a constructor property equal to the Observable constructor that does not contain a subscribe method or otherwise behave like an Observable. Should we use a brand check to confirm that the input to Observable.from is indeed a species of Observable?

zenparsing commented 7 years ago

FWIW, I think that Observable.from calls @@observable on the input and then compares that object's constructor to the this value. I think the idea originally was that the brand check was unnecessary since the object is a result of calling @@observable.

benjamingr commented 7 years ago

This seems easy to spoof.

Why is that a bad thing? It gives another point where people can hook and it won't affect real users in obvious ways.

RangerMauve commented 7 years ago

The only issue I could see is if somebody creates a misbehaving observable, but if someone's code isn't working, people won't use it. Like a misbehaving .then with promise implementations.

benjamingr commented 7 years ago

Like a misbehaving .then with promise implementations.

Yeah, a promise is complicated in handling then assimilation (and handles cases like "what is it a getter that throws the second time it's accessed") because promises were designed around working with faulty implementations. ES-Observable semantics don't have to deal with that.

We can just do the simple thing.

ljharb commented 6 years ago

Promises do indeed have brand checks; I would expect Observables to have a brand as well.

However, like Array.from, I would expect Observable.from to behave differently based on the presence of the brand, but not to require it.

zenparsing commented 6 years ago

Promises do indeed have brand checks; I would expect Observables to have a brand as well.

My understanding is that the brand checks in Promises are there in order to support certain security guarantees. It's not clear to me (yet) that Observable is required to have the same guarantees, or if a brand check would be sufficient for such guarantees.

Also, does Array.from have a brand check? I didn't see one.

ljharb commented 6 years ago

@zenparsing ah, good call, Array.from doesn't - for arrays it just uses the Array Symbol.iterator.

In that case Observable.from definitely would be able to use the protocol, and not brand-check, but I'd still expect brand checking somewhere. Part of the agreement for keeping Symbol.toStringTag in the spec in ES6 was that every new type in the spec would always provide some mechanism for brand checking, even if that was via try/catch - generally, it's some instance method or accessor that throws when the brand is missing. This probably is the wrong issue to bring this up on, but I'd want it resolved before stage progression.

zenparsing commented 6 years ago

What is our feeling about Symbol.observable? I know it has been helpful as a way for different libraries in the wild to interoperate, but I find it to be a bit of a pain. I think every time that I create an "observable" abstraction, I want to add a "subscribe" method anyway.

I argued pretty strongly for it but now I'm not so sure...

@ljharb @jhusain What would you think about removing the whole Symbol.observable call? Instead of using @@observable, Observable.from would instead just look for a "subscribe" method.

A working example is here: https://github.com/zenparsing/zen-observable/blob/651c8159ac0cc17479d3c86f890cc74831b7b8f4/src/Observable.js#L313.

ljharb commented 6 years ago

That seems more ergonomic, however, that seems to echo the choice of using “then” on Promise - which many feel was unfortunate, because of collisions.

zenparsing commented 6 years ago

@ljharb Right, I was of that opinion as well. However, I don't think it will be anywhere near as controversial for observables since (a) observables don't auto-flatten like promises, and (b) we are not really aiming for language-level integration.

jhusain commented 6 years ago

Having run into at least one situation where I didn’t want to use the name “then” in a library at the risk of accidental coercion into Promises, I would still favor symbols.

ljharb commented 6 years ago

I’m not sure what you mean about the last part; i very much think Observable needs language-level integration (unless you mean syntax?)

zenparsing commented 6 years ago

@jhusain In what situations might we get accidental coercion to Observable? Right now, we only do coercion as part of from (which is pretty explicit). With promises, on the other hand, coercion happens implicitly with every map.

@ljharb Yes, I mean we are not aiming for syntax.

benlesh commented 6 years ago

What would you think about removing the whole Symbol.observable call? Instead of using @@observable, Observable.from would instead just look for a "subscribe" method.

I would be against this. There are so many APIs in existence with subscribe methods on them. Redux stores, for example. Using a Symbol is a great pattern we can borrow from iterables, which are spiritually linked to observables.

It's bad enough that then is basically a reserved method name now. i'd hate to do that with subscribe also.

But on the flip side: Most people assume something is an Observable when they see a subscribe call... but that's in my clearly lop-sided experience.

jhusain commented 6 years ago

Additionally many userland Observables don’t have the same subscribe API signature. There will be plenty of old Observables in the wild with a subscribe() that expects Observers to have an onNext() method for example.

Sent from my iPhone

On Jan 30, 2018, at 9:37 AM, Ben Lesh notifications@github.com wrote:

What would you think about removing the whole Symbol.observable call? Instead of using @@observable, Observable.from would instead just look for a "subscribe" method.

I would be against this. There are so many APIs in existence with subscribe methods on them. Redux stores, for example. Using a Symbol is a great pattern we can borrow from iterables, which are spiritually linked to observables.

It's bad enough that then is basically a reserved method name now. i'd hate to do that with subscribe also.

But on the flip side: Most people assume something is an Observable when they see a subscribe call... but that's in my clearly lop-sided experience.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

zenparsing commented 6 years ago

@jhusain @benlesh Good arguments. Thanks!

zenparsing commented 6 years ago

Coming back to the original question, I think this depends on whether or not we want Observable.from to provide protection from producer interference.

Assuming that Observable.from is the built-in Observable.from, should calling Observable.from guarantee that the resulting object's subscribe method will not throw, for instance?