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

feat(store): add observable proposal interop to store #1632

Merged
merged 1 commit into from Apr 19, 2016

Conversation

@benlesh
Contributor

benlesh commented Apr 18, 2016

  • Adds Symbol.observable method to the store that returns a generic observable
  • Adds tests to ensure interoperability. (rxjs 5 was used for a simple integration test, and
    is a dev only dependency)

closes #1631

@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Apr 18, 2016

Contributor

No more "RxJS vs Redux"!

Ducks for everyone! Ducks all around!

Contributor

benlesh commented Apr 18, 2016

No more "RxJS vs Redux"!

Ducks for everyone! Ducks all around!

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 18, 2016

Collaborator

This looks great to me. I’d leave it open for a few days in case somebody else has any feedback.

Collaborator

gaearon commented Apr 18, 2016

This looks great to me. I’d leave it open for a few days in case somebody else has any feedback.

@tomenden

This comment has been minimized.

Show comment
Hide comment
@tomenden

tomenden Apr 18, 2016

This is great

This is great

Show outdated Hide outdated src/utils/Symbol_observable.js
$$observable = '@@observable'
}
export default $$observable

This comment has been minimized.

@timdorr

timdorr Apr 18, 2016

Member

Can this shim be maintained elsewhere? Much like we've swapped out built-in utilities for lodash, I don't think it's good if we try to maintain this shim code internally. Especially if the spec changes as it makes its way through the proposal process.

@timdorr

timdorr Apr 18, 2016

Member

Can this shim be maintained elsewhere? Much like we've swapped out built-in utilities for lodash, I don't think it's good if we try to maintain this shim code internally. Especially if the spec changes as it makes its way through the proposal process.

This comment has been minimized.

@benlesh

benlesh Apr 18, 2016

Contributor

I'm 50/50 on this, personally. On one hand, it's not a lot of code, and there's the ol' left-pad issue. On the other hand, I tend to agree it could be centralized somewhere. I know that @sindresorhus has a ponyfill for this, but I've seen a couple of bugs in it and I haven't had the time to reach out to collaborate with him (I don't actually know him at all). Perhaps later this afternoon I'll put in a PR on his repository and if that doesn't fly I can always publish something elsewhere, ReactiveX perhaps.

@benlesh

benlesh Apr 18, 2016

Contributor

I'm 50/50 on this, personally. On one hand, it's not a lot of code, and there's the ol' left-pad issue. On the other hand, I tend to agree it could be centralized somewhere. I know that @sindresorhus has a ponyfill for this, but I've seen a couple of bugs in it and I haven't had the time to reach out to collaborate with him (I don't actually know him at all). Perhaps later this afternoon I'll put in a PR on his repository and if that doesn't fly I can always publish something elsewhere, ReactiveX perhaps.

This comment has been minimized.

@benlesh

benlesh Apr 18, 2016

Contributor

In the meantime, this could ship as-is, if blessed. And I'll be happy to come back and make a change to use a separate module. I'll leave that up to @gaearon.

FWIW, Falcor has similar concerns. But it was determined that it's not a huge deal.

@benlesh

benlesh Apr 18, 2016

Contributor

In the meantime, this could ship as-is, if blessed. And I'll be happy to come back and make a change to use a separate module. I'll leave that up to @gaearon.

FWIW, Falcor has similar concerns. But it was determined that it's not a huge deal.

This comment has been minimized.

@sindresorhus

sindresorhus Apr 18, 2016

@blesh Regardless of it being included here, I would appreciate a issue/PR about the bugs 😀 We use the ponyfill in AVA, so it's important that it works correctly.

@sindresorhus

sindresorhus Apr 18, 2016

@blesh Regardless of it being included here, I would appreciate a issue/PR about the bugs 😀 We use the ponyfill in AVA, so it's important that it works correctly.

This comment has been minimized.

@timdorr

timdorr Apr 18, 2016

Member

BTW, for those wondering (and observing 😄), the aforementioned ponyfill is here: https://github.com/sindresorhus/symbol-observable

@timdorr

timdorr Apr 18, 2016

Member

BTW, for those wondering (and observing 😄), the aforementioned ponyfill is here: https://github.com/sindresorhus/symbol-observable

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr Apr 18, 2016

Member

Only question I have is how stable the Observable proposal is going to be. Is it likely to remain unchanged? If so, then we should be A-OK here. If there is some potential for change in the future, that might be a headache to keep in mind on the horizon.

Member

timdorr commented Apr 18, 2016

Only question I have is how stable the Observable proposal is going to be. Is it likely to remain unchanged? If so, then we should be A-OK here. If there is some potential for change in the future, that might be a headache to keep in mind on the horizon.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 18, 2016

Collaborator

I say let’s ship it but not document this until it shakes out a little?

Collaborator

gaearon commented Apr 18, 2016

I say let’s ship it but not document this until it shakes out a little?

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr Apr 18, 2016

Member

A secret package of awesomeness 😄

Let's see how Ben's PR shakes out and then merge it in depending on the result there.

Member

timdorr commented Apr 18, 2016

A secret package of awesomeness 😄

Let's see how Ben's PR shakes out and then merge it in depending on the result there.

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Apr 18, 2016

Collaborator

Weeeee this is cool

Collaborator

acdlite commented Apr 18, 2016

Weeeee this is cool

@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Apr 18, 2016

Contributor

Only question I have is how stable the Observable proposal is going to be.

This particular piece is likely to remain pretty solid. It's implemented in 3-4 major reactive libraries, and I don't see it changing any time soon. Even if the observable proposal flops at the TC39, I'd expect this piece to live on in libraries like RxJS, etc.

Contributor

benlesh commented Apr 18, 2016

Only question I have is how stable the Observable proposal is going to be.

This particular piece is likely to remain pretty solid. It's implemented in 3-4 major reactive libraries, and I don't see it changing any time soon. Even if the observable proposal flops at the TC39, I'd expect this piece to live on in libraries like RxJS, etc.

@jbaxleyiii jbaxleyiii referenced this pull request Apr 18, 2016

Merged

Observable API #119

2 of 2 tasks complete
@molily

This comment has been minimized.

Show comment
Hide comment
@molily

molily Apr 18, 2016

Even if the observable proposal flops at the TC39, I'd expect this piece to live on in libraries like RxJS, etc.

Sorry to jump in here, I’m totally ignorant about these libraries. But as a user of Redux I do not expect Redux to modify global objects (Symbol.observable = …) via a utility file when the property is only defined in a TC39 stage 1 proposal. For the purpose of this PR, the line Symbol.observable = $$observable is negligible, isn’t it?

molily commented Apr 18, 2016

Even if the observable proposal flops at the TC39, I'd expect this piece to live on in libraries like RxJS, etc.

Sorry to jump in here, I’m totally ignorant about these libraries. But as a user of Redux I do not expect Redux to modify global objects (Symbol.observable = …) via a utility file when the property is only defined in a TC39 stage 1 proposal. For the purpose of this PR, the line Symbol.observable = $$observable is negligible, isn’t it?

@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Apr 18, 2016

Contributor

For the purpose of this PR, the line Symbol.observable = $$observable is negligible, isn’t it?

Actually no, it has real purpose. In the event that the engine has Symbol, but not Symbol.for, then a Symbol is made, which is completely unique. It then needs to be placed on Symbol.observable in order for it to be found and used by other libraries that care about it (Falcor, RxJS, Bacon, Kefir, etc).

I do not expect Redux to modify global objects

This is a bit of a doom-and-gloom way to say it, the tone seems to want to incite fear. The reality is almost every library that you can use with a <script> tag is modifying at least window. Is there a specific concern around this modification? Are you concerned that other libraries are leveraging Symbol.observable in some other way?

Contributor

benlesh commented Apr 18, 2016

For the purpose of this PR, the line Symbol.observable = $$observable is negligible, isn’t it?

Actually no, it has real purpose. In the event that the engine has Symbol, but not Symbol.for, then a Symbol is made, which is completely unique. It then needs to be placed on Symbol.observable in order for it to be found and used by other libraries that care about it (Falcor, RxJS, Bacon, Kefir, etc).

I do not expect Redux to modify global objects

This is a bit of a doom-and-gloom way to say it, the tone seems to want to incite fear. The reality is almost every library that you can use with a <script> tag is modifying at least window. Is there a specific concern around this modification? Are you concerned that other libraries are leveraging Symbol.observable in some other way?

@glenjamin

This comment has been minimized.

Show comment
Hide comment
@glenjamin

glenjamin Apr 18, 2016

Would it be feasible to ship this initially as a module, and then fold into core if it's popular / vital / stable / enough?

As far as I can tell it only depends on two redux API methods, subscribe and getState, both of which are part of the public API.

Something like this that could be referenced in the official docs should be doable?

import { observeStore } from 'redux-observable';
observeStore(store);

Would it be feasible to ship this initially as a module, and then fold into core if it's popular / vital / stable / enough?

As far as I can tell it only depends on two redux API methods, subscribe and getState, both of which are part of the public API.

Something like this that could be referenced in the official docs should be doable?

import { observeStore } from 'redux-observable';
observeStore(store);
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 18, 2016

Collaborator

Since the purpose is interop, I think shipping it as a separate module kinda defeats the purpose. It’s like if Immutable Maps weren’t Iterables and required a separate lib.

I do think we want to add this. However I would vastly prefer this be an external polyfill so discussions like this can happen on its issue tracker while we just declare compat 😄 .

Collaborator

gaearon commented Apr 18, 2016

Since the purpose is interop, I think shipping it as a separate module kinda defeats the purpose. It’s like if Immutable Maps weren’t Iterables and required a separate lib.

I do think we want to add this. However I would vastly prefer this be an external polyfill so discussions like this can happen on its issue tracker while we just declare compat 😄 .

@molily

This comment has been minimized.

Show comment
Hide comment
@molily

molily Apr 18, 2016

@blesh

It then needs to be placed on Symbol.observable in order for it to be found and used by other libraries …

Thanks for the explanation. When this is about interoperability without explicitly passing the symbol to other libs, creating a global makes sense.

This is a bit of a doom-and-gloom way to say it, the tone seems to want to incite fear.

That wasn’t my intention. :-D

molily commented Apr 18, 2016

@blesh

It then needs to be placed on Symbol.observable in order for it to be found and used by other libraries …

Thanks for the explanation. When this is about interoperability without explicitly passing the symbol to other libs, creating a global makes sense.

This is a bit of a doom-and-gloom way to say it, the tone seems to want to incite fear.

That wasn’t my intention. :-D

@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Apr 19, 2016

Contributor

@gaearon FYI: I've updated this to use a "small module" (THANK YOU @sindresorhus!!!!) to get the reference to Symbol.observable.

Contributor

benlesh commented Apr 19, 2016

@gaearon FYI: I've updated this to use a "small module" (THANK YOU @sindresorhus!!!!) to get the reference to Symbol.observable.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 19, 2016

Collaborator

y u no lint @blesh

Collaborator

gaearon commented Apr 19, 2016

y u no lint @blesh

Show outdated Hide outdated src/createStore.js
@@ -198,6 +199,26 @@ export default function createStore(reducer, initialState, enhancer) {
dispatch({ type: ActionTypes.INIT })
}
/**
* Interop point for observable libraries

This comment has been minimized.

@gaearon

gaearon Apr 19, 2016

Collaborator

Let’s format these a little more consistent with the rest of JSDoc in this file:

  • End sentences with period.
  • Start parameter descriptions with a capital letter.
  • I would rather avoid the awkward hanging link and instead split this over two lines, i.e. see the observable proposal: (newline) http://...
@gaearon

gaearon Apr 19, 2016

Collaborator

Let’s format these a little more consistent with the rest of JSDoc in this file:

  • End sentences with period.
  • Start parameter descriptions with a capital letter.
  • I would rather avoid the awkward hanging link and instead split this over two lines, i.e. see the observable proposal: (newline) http://...
Show outdated Hide outdated test/createStore.spec.js
}
expect(typeof store[key]).toBe('function')

This comment has been minimized.

@gaearon

gaearon Apr 19, 2016

Collaborator

Nitpick: let’s make it a little denser. We generally follow this pattern:

some setup

line 1
line 2
line 3
related expect call

line 4
line 5
related expect call

...
@gaearon

gaearon Apr 19, 2016

Collaborator

Nitpick: let’s make it a little denser. We generally follow this pattern:

some setup

line 1
line 2
line 3
related expect call

line 4
line 5
related expect call

...

This comment has been minimized.

@benlesh

benlesh Apr 19, 2016

Contributor

Here we can probably use symbol-observable now instead of having all of this code to figure out what to use.

@benlesh

benlesh Apr 19, 2016

Contributor

Here we can probably use symbol-observable now instead of having all of this code to figure out what to use.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 19, 2016

Collaborator

Thank you for your work on this, and special thanks to @sindresorhus.
Left a few minor nits and this is ready to go.

Collaborator

gaearon commented Apr 19, 2016

Thank you for your work on this, and special thanks to @sindresorhus.
Left a few minor nits and this is ready to go.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 19, 2016

Collaborator

@blesh I’m happy to do nits myself if you don’t have the time. Just let me know.

Collaborator

gaearon commented Apr 19, 2016

@blesh I’m happy to do nits myself if you don’t have the time. Just let me know.

@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Apr 19, 2016

Contributor

@gaearon I can do them. I just got these messages. No problem.

Contributor

benlesh commented Apr 19, 2016

@gaearon I can do them. I just got these messages. No problem.

feat(store): add observable proposal interop to store
- Adds dependency on `symbol-observable` to pull in `Symbol.observable`
- Adds `Symbol.observable` method to the store that returns a generic observable
- Adds comprehensive tests to ensure interoperability. (rxjs 5 was used for a simple integration test, and
  is a dev only dependency)

closes #1631
@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Apr 19, 2016

Contributor

@gaearon I've made the requested changes (I think), and I've added some additional tests to ensure the functionality is solid.

I've also flattened the changes into a single commit for cleanliness.

Contributor

benlesh commented Apr 19, 2016

@gaearon I've made the requested changes (I think), and I've added some additional tests to ensure the functionality is solid.

I've also flattened the changes into a single commit for cleanliness.

@gaearon gaearon merged commit 64551cb into reduxjs:master Apr 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 19, 2016

Collaborator

Looking great. Will cut a release tomorrow.

Collaborator

gaearon commented Apr 19, 2016

Looking great. Will cut a release tomorrow.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 19, 2016

Collaborator

Should this bump minor? I think so.

Collaborator

gaearon commented Apr 19, 2016

Should this bump minor? I think so.

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Apr 19, 2016

Collaborator

Yeah it's a new feature, so it should bump minor. Especially since extensions may depend on it.

Collaborator

acdlite commented Apr 19, 2016

Yeah it's a new feature, so it should bump minor. Especially since extensions may depend on it.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 20, 2016

Collaborator

Out in 3.5.0.

Collaborator

gaearon commented Apr 20, 2016

Out in 3.5.0.

@SimenB

This comment has been minimized.

Show comment
Hide comment
@SimenB

SimenB Apr 23, 2016

Contributor

This explodes in IE8 with Expected identifier, on the for of this line https://github.com/blesh/symbol-observable/blob/ada343f566522f9b2dc3046dda0364e44c0138fc/ponyfill.js#L11, as it's a reserved word. es3ify fixes it, but shouldn't be necessary for a consumer standpoint.

EDIT: PR here benlesh/symbol-observable#6

Should there be some sort of automated testing, at least for syntax, until the big 4.0 where you drop support for it? Isn't the first time IE8 support is broken.

Maybe manually just running a umd build build with and without es3ify or something, and expect no diff? EDIT2: I see UMD is already ran through es3ify, so might be difficult to detect if a dependency breaks. And new dependencies are pretty rare, so might not be worth the effort to test for it.

I start a new job in a week, it'll be so good to stop caring about IE8! 😆

Contributor

SimenB commented Apr 23, 2016

This explodes in IE8 with Expected identifier, on the for of this line https://github.com/blesh/symbol-observable/blob/ada343f566522f9b2dc3046dda0364e44c0138fc/ponyfill.js#L11, as it's a reserved word. es3ify fixes it, but shouldn't be necessary for a consumer standpoint.

EDIT: PR here benlesh/symbol-observable#6

Should there be some sort of automated testing, at least for syntax, until the big 4.0 where you drop support for it? Isn't the first time IE8 support is broken.

Maybe manually just running a umd build build with and without es3ify or something, and expect no diff? EDIT2: I see UMD is already ran through es3ify, so might be difficult to detect if a dependency breaks. And new dependencies are pretty rare, so might not be worth the effort to test for it.

I start a new job in a week, it'll be so good to stop caring about IE8! 😆

@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Apr 23, 2016

Contributor

@SimenB there will be a new version of symbol-observable this weekend with the fix.

Contributor

benlesh commented Apr 23, 2016

@SimenB there will be a new version of symbol-observable this weekend with the fix.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 23, 2016

Collaborator

@SimenB I sympathize with your concerns but I don’t think any automated testing is worth the effort here. Technically we support IE8, and regressions aren’t good, but we try to fix them as they are reported. When ES3 plugins were broken in Babel 6 for three months, many libraries had ES3 issues, and I think at this point whoever cares about it should be wary of updating dependencies and take care of ES3-ifying the output themselves. I don’t mean to come across as dismissive—just saying there’s been a ton of IE8 breakage in the ecosystem, and we’re hardly the worst offenders. I agree we should fix it of course 😄 .

@blesh Thanks!

Collaborator

gaearon commented Apr 23, 2016

@SimenB I sympathize with your concerns but I don’t think any automated testing is worth the effort here. Technically we support IE8, and regressions aren’t good, but we try to fix them as they are reported. When ES3 plugins were broken in Babel 6 for three months, many libraries had ES3 issues, and I think at this point whoever cares about it should be wary of updating dependencies and take care of ES3-ifying the output themselves. I don’t mean to come across as dismissive—just saying there’s been a ton of IE8 breakage in the ecosystem, and we’re hardly the worst offenders. I agree we should fix it of course 😄 .

@blesh Thanks!

@SimenB

This comment has been minimized.

Show comment
Hide comment
@SimenB

SimenB Apr 23, 2016

Contributor

Always shrinkwrapping sucks, but it might make sense yeah. I'm out in a week, so not a huge bother for me personally, though!

ES3 plugins are still somewhat broken in babel 6 btw, so that's fun

Contributor

SimenB commented Apr 23, 2016

Always shrinkwrapping sucks, but it might make sense yeah. I'm out in a week, so not a huge bother for me personally, though!

ES3 plugins are still somewhat broken in babel 6 btw, so that's fun

@benlesh

This comment has been minimized.

Show comment
Hide comment
@benlesh

benlesh Apr 24, 2016

Contributor

@SimenB symbol-observable@0.2.3 is released with this change.

Contributor

benlesh commented Apr 24, 2016

@SimenB symbol-observable@0.2.3 is released with this change.

benlesh added a commit to benlesh/redux that referenced this pull request Apr 24, 2016

fix(Symbol.observable): update symbol-observable to fix IE8 issue
This updates symbol-observable dependency to be 0.2.3 or higher in order to fix an issue where legacy browsers did not like
Symbol.for statement inside of the ponyfill

related #1632
related #774

gaearon added a commit that referenced this pull request Apr 24, 2016

fix(Symbol.observable): update symbol-observable to fix IE8 issue
This updates symbol-observable dependency to be 0.2.3 or higher in order to fix an issue where legacy browsers did not like
Symbol.for statement inside of the ponyfill

related #1632
related #774
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 24, 2016

Collaborator

Redux 3.5.2 enforces the updated symbol-observable. Thanks @blesh.

Collaborator

gaearon commented Apr 24, 2016

Redux 3.5.2 enforces the updated symbol-observable. Thanks @blesh.

@@ -198,6 +199,49 @@ export default function createStore(reducer, initialState, enhancer) {
dispatch({ type: ActionTypes.INIT })
}
/**

This comment has been minimized.

@benjamingr

benjamingr Jun 25, 2016

Hey @blesh , I wanted to add Symbol.observable to mobx and I was wondering if you felt like putting this code on npm somewhere because I'd feel stupid copy pasting it or rewriting it.

@benjamingr

benjamingr Jun 25, 2016

Hey @blesh , I wanted to add Symbol.observable to mobx and I was wondering if you felt like putting this code on npm somewhere because I'd feel stupid copy pasting it or rewriting it.

This comment has been minimized.

@benlesh

benlesh Jun 27, 2016

Contributor

I would, but I don't know that it's really that globally applicable to everyone. At least the part that adapts some arbitrary type (in this case a redux store) into an Observable.

Is there an issue on MobX I can check out?

@benlesh

benlesh Jun 27, 2016

Contributor

I would, but I don't know that it's really that globally applicable to everyone. At least the part that adapts some arbitrary type (in this case a redux store) into an Observable.

Is there an issue on MobX I can check out?

This comment has been minimized.

@benjamingr

benjamingr Jun 27, 2016

No, but I talked to @mweststrate directly and he was interested in the idea. mobx is based on observables but they are depedency tracking and very different (and much simpler) than rxjs ones.

I now write a lot of mobx -> rxjs -> mobx code and I figured it could be cool for Rx to consume mobx observables directly.

@benjamingr

benjamingr Jun 27, 2016

No, but I talked to @mweststrate directly and he was interested in the idea. mobx is based on observables but they are depedency tracking and very different (and much simpler) than rxjs ones.

I now write a lot of mobx -> rxjs -> mobx code and I figured it could be cool for Rx to consume mobx observables directly.

This comment has been minimized.

@chicoxyzzy

chicoxyzzy Jun 27, 2016

Contributor

@blesh @benjamingr there is mobxjs/mobx#169 but I stopped my experiments with mobx so I didn't made any progress. Also there is an interop you @benjamingr might find useful https://github.com/chicoxyzzy/rx-mobx

@chicoxyzzy

chicoxyzzy Jun 27, 2016

Contributor

@blesh @benjamingr there is mobxjs/mobx#169 but I stopped my experiments with mobx so I didn't made any progress. Also there is an interop you @benjamingr might find useful https://github.com/chicoxyzzy/rx-mobx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment