search

tc39 / proposal-observable

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

Additional operators and unit tests #189

Open jrobinson01 opened 6 years ago

jrobinson01 commented 6 years ago

Apologies for the title. This is probably going to be a bit of a hodge podge and more discussion than "issue".

First, I added some new operators, and some construction type things... like fromEvent, fromPromise, etc. https://github.com/jrobinson01/proposal-observable

I was going to create a PR but I also wanted to write some tests first. I started by including the existing future/filter spec and had high hopes of making that pass. I haven't gotten very far, which leads to my first questions.

The filter spec has a few tests that I'm not sure how to make pass. These are all to do with return values: https://github.com/tc39/proposal-observable/blob/master/test/future/filter.js#L73 https://github.com/tc39/proposal-observable/blob/master/test/future/filter.js#L94 https://github.com/tc39/proposal-observable/blob/master/test/future/filter.js#L115 https://github.com/tc39/proposal-observable/blob/master/test/future/filter.js#L136

Given this filter implementation: https://github.com/jrobinson01/proposal-observable/blob/master/src/Observable.js#L369 what is missing to satisfy these tests?

Concerning the test for complete: https://github.com/tc39/proposal-observable/blob/master/test/future/filter.js#L120

I haven't been able to get either of those to pass. I'm a bit confused by "Complete values are forwarded". Did we decide what complete should and should not do? Should it pass a value at all? If so, what value?

Finally, some more general questions...

Is this proposal still active?

I see mention of filter and map existing in this proposal at one point. I assume they were since removed but am not sure I understand why?

Is it worth creating a PR if I can get test coverage?

I feel like I had more questions but I guess this is plenty for one issue.

appsforartists commented 6 years ago

I think operators are out-of-scope for this proposal - it's intentionally a limited set to keep the scope of the initial proposal reasonable, to avoid bikeshedding on what should/shouldn't be included.

I'd be curious to see a blessed extensibility method (like pipe). However, turning this proposal into RxJS risks bloating it, distracting from the core pattern. Considering @benlesh and @jhusain are both active here, it's fair to say they are aware of RxJS's features and can thoughtfully include/exclude them as appropriate.

jrobinson01 commented 6 years ago

I think operators are out-of-scope for this proposal - it's intentionally a limited set to keep the scope of the initial proposal reasonable, to avoid bikeshedding on what should/shouldn't be included.

That's pretty much what I figured. I think settling on the common array operators though would be a nice middle ground. Some of the other stuff I've added might be questionable (fromEvent is dom element specific, scan is a weird name and only exists because reduce I modeled after rxjs (I think, it was months ago!) and then found it didn't do what I expected).