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

Additional operators and unit tests #189

Open
jrobinson01 opened this issue Jun 20, 2018 · 2 comments
Open

Additional operators and unit tests #189

jrobinson01 opened this issue Jun 20, 2018 · 2 comments

Comments

@jrobinson01
Copy link

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
Copy link
Contributor

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
Copy link
Author

jrobinson01 commented Jun 20, 2018

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).

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

2 participants