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

benlesh · 2016-04-18T16:36:41Z

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 · 2016-04-18T16:38:02Z

No more "RxJS vs Redux"!

Ducks for everyone! Ducks all around!

gaearon · 2016-04-18T16:45:34Z

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

tomenden · 2016-04-18T17:10:42Z

This is great

timdorr · 2016-04-18T17:42:36Z

src/utils/Symbol_observable.js

+  $$observable = '@@observable'
+}
+
+export default $$observable


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.

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.

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.

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

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

timdorr · 2016-04-18T17:44:12Z

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 · 2016-04-18T18:07:31Z

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

timdorr · 2016-04-18T18:12:51Z

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 · 2016-04-18T18:30:28Z

Weeeee this is cool

benlesh · 2016-04-18T18:34:36Z

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.

molily · 2016-04-18T20:01:32Z

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 · 2016-04-18T20:10:03Z

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 · 2016-04-18T20:14:12Z

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 · 2016-04-18T20:15:48Z

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 · 2016-04-18T20:26:53Z

@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 · 2016-04-19T00:50:52Z

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

gaearon · 2016-04-19T01:03:24Z

y u no lint @Blesh

gaearon · 2016-04-19T01:05:58Z

src/createStore.js

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

+  /**
+   * Interop point for observable libraries


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 · 2016-04-19T01:08:28Z

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

gaearon · 2016-04-19T14:52:22Z

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

benlesh · 2016-04-19T16:24:39Z

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

- 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 reduxjs#1631

benlesh · 2016-04-19T17:55:55Z

@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 · 2016-04-19T21:44:12Z

Looking great. Will cut a release tomorrow.

gaearon · 2016-04-19T21:44:36Z

Should this bump minor? I think so.

acdlite · 2016-04-19T23:46:09Z

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

gaearon · 2016-04-20T00:06:12Z

Out in 3.5.0.

SimenB · 2016-04-23T05:55:09Z

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 · 2016-04-23T15:12:50Z

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

gaearon · 2016-04-23T17:43:00Z

@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 · 2016-04-23T17:56:15Z

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 · 2016-04-24T06:11:02Z

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

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 reduxjs#1632 related reduxjs#774

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 · 2016-04-24T12:27:06Z

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

benjamingr · 2016-06-25T09:43:10Z

src/createStore.js

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

+  /**


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.

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?

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.

@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

benlesh force-pushed the make-observable branch from 5271b87 to 498e6d0 Compare April 18, 2016 16:44

timdorr reviewed Apr 18, 2016
View reviewed changes

jbaxleyiii mentioned this pull request Apr 18, 2016

Observable API apollographql/apollo-client#119

Merged

2 tasks

stubailo mentioned this pull request Apr 18, 2016

Switch to Redux observable implementation apollographql/apollo-client#123

Closed

gaearon reviewed Apr 19, 2016
View reviewed changes

benlesh force-pushed the make-observable branch from 164aa6d to f99c2ba Compare April 19, 2016 17:47

benlesh force-pushed the make-observable branch from e549db5 to e5bcbbe Compare April 19, 2016 17:54

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

rubyboy mentioned this pull request Apr 21, 2016

Add observables via select() InfomediaLtd/angular2-redux#12

Merged

benlesh mentioned this pull request Apr 24, 2016

fix(Symbol.observable): update symbol-observable to fix IE8 issue #1659

Merged

gaearon mentioned this pull request May 11, 2016

chore(dependencies): update symbol-observable to 0.2.4 #1663

Merged

pkese mentioned this pull request May 11, 2016

Document how people can benefit from observable spec interop #1718

Closed

SimenB mentioned this pull request May 12, 2016

Check ES3 syntax compatibility #1720

Merged

lacolaco mentioned this pull request May 23, 2016

discuss: ES Observable Interop angular/angular#8776

Closed

chicoxyzzy mentioned this pull request Jun 12, 2016

Merge rx-recompose into main project acdlite/recompose#196

Merged

3 tasks

benjamingr reviewed Jun 25, 2016
View reviewed changes

snyk-bot mentioned this pull request Mar 30, 2020

[Snyk] Upgrade redux from 3.2.1 to 3.7.2 dmitriz/scalable-frontend-with-elm-or-redux#5

Merged

Methuselah96 mentioned this pull request Jan 12, 2022

Breaking changes after updating extension to v3 reduxjs/redux-devtools#1002

Closed

andrewbrereton mentioned this pull request Jun 1, 2024

[Snyk] Upgrade redux from 3.0.4 to 3.7.2 andrewbrereton/react-redux-universal-hot-example#4

Open

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

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

Conversation

benlesh commented Apr 18, 2016

benlesh commented Apr 18, 2016

gaearon commented Apr 18, 2016

tomenden commented Apr 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timdorr commented Apr 18, 2016

gaearon commented Apr 18, 2016

timdorr commented Apr 18, 2016

acdlite commented Apr 18, 2016

benlesh commented Apr 18, 2016

molily commented Apr 18, 2016

benlesh commented Apr 18, 2016 • edited by gaearon Loading

glenjamin commented Apr 18, 2016

gaearon commented Apr 18, 2016

molily commented Apr 18, 2016 • edited Loading

benlesh commented Apr 19, 2016

gaearon commented Apr 19, 2016

gaearon Apr 19, 2016 • edited Loading

Choose a reason for hiding this comment

gaearon commented Apr 19, 2016

gaearon commented Apr 19, 2016

benlesh commented Apr 19, 2016

benlesh commented Apr 19, 2016

gaearon commented Apr 19, 2016

gaearon commented Apr 19, 2016

acdlite commented Apr 19, 2016

gaearon commented Apr 20, 2016

SimenB commented Apr 23, 2016 • edited Loading

benlesh commented Apr 23, 2016

gaearon commented Apr 23, 2016

SimenB commented Apr 23, 2016 • edited Loading

benlesh commented Apr 24, 2016

gaearon commented Apr 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chicoxyzzy Jun 27, 2016 • edited Loading

Choose a reason for hiding this comment

benlesh commented Apr 18, 2016 •

edited by gaearon

Loading

molily commented Apr 18, 2016 •

edited

Loading

gaearon Apr 19, 2016 •

edited

Loading

SimenB commented Apr 23, 2016 •

edited

Loading

SimenB commented Apr 23, 2016 •

edited

Loading

chicoxyzzy Jun 27, 2016 •

edited

Loading