-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,8 @@ import expect from 'expect' | |
import { createStore, combineReducers } from '../src/index' | ||
import { addTodo, dispatchInMiddle, throwError, unknownAction } from './helpers/actionCreators' | ||
import * as reducers from './helpers/reducers' | ||
import * as Rx from 'rxjs' | ||
import $$observable from 'symbol-observable' | ||
|
||
describe('createStore', () => { | ||
it('exposes the public API', () => { | ||
|
@@ -610,4 +612,118 @@ describe('createStore', () => { | |
store.subscribe(undefined) | ||
).toThrow() | ||
}) | ||
|
||
describe('Symbol.observable interop point', () => { | ||
it('should exist', () => { | ||
const store = createStore(() => {}) | ||
expect(typeof store[$$observable]).toBe('function') | ||
}) | ||
|
||
describe('returned value', () => { | ||
it('should be subscribable', () => { | ||
const store = createStore(() => {}) | ||
const obs = store[$$observable]() | ||
expect(typeof obs.subscribe).toBe('function') | ||
}) | ||
|
||
it('should throw a TypeError if an observer object is not supplied to subscribe', () => { | ||
const store = createStore(() => {}) | ||
const obs = store[$$observable]() | ||
|
||
expect(function () { | ||
obs.subscribe() | ||
}).toThrow() | ||
|
||
expect(function () { | ||
obs.subscribe(() => {}) | ||
}).toThrow() | ||
|
||
expect(function () { | ||
obs.subscribe({}) | ||
}).toNotThrow() | ||
}) | ||
|
||
it('should return a subscription object when subscribed', () => { | ||
const store = createStore(() => {}) | ||
const obs = store[$$observable]() | ||
const sub = obs.subscribe({}) | ||
expect(typeof sub.unsubscribe).toBe('function') | ||
}) | ||
}) | ||
|
||
it('should pass an integration test with no unsubscribe', () => { | ||
function foo(state = 0, action) { | ||
return action.type === 'foo' ? 1 : state | ||
} | ||
|
||
function bar(state = 0, action) { | ||
return action.type === 'bar' ? 2 : state | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we can probably use |
||
const store = createStore(combineReducers({ foo, bar })) | ||
const observable = store[$$observable]() | ||
const results = [] | ||
|
||
observable.subscribe({ | ||
next(state) { | ||
results.push(state) | ||
} | ||
}) | ||
|
||
store.dispatch({ type: 'foo' }) | ||
store.dispatch({ type: 'bar' }) | ||
|
||
expect(results).toEqual([ { foo: 0, bar: 0 }, { foo: 1, bar: 0 }, { foo: 1, bar: 2 } ]) | ||
}) | ||
|
||
it('should pass an integration test with an unsubscribe', () => { | ||
function foo(state = 0, action) { | ||
return action.type === 'foo' ? 1 : state | ||
} | ||
|
||
function bar(state = 0, action) { | ||
return action.type === 'bar' ? 2 : state | ||
} | ||
|
||
const store = createStore(combineReducers({ foo, bar })) | ||
const observable = store[$$observable]() | ||
const results = [] | ||
|
||
const sub = observable.subscribe({ | ||
next(state) { | ||
results.push(state) | ||
} | ||
}) | ||
|
||
store.dispatch({ type: 'foo' }) | ||
sub.unsubscribe() | ||
store.dispatch({ type: 'bar' }) | ||
|
||
expect(results).toEqual([ { foo: 0, bar: 0 }, { foo: 1, bar: 0 } ]) | ||
}) | ||
|
||
it('should pass an integration test with a common library (RxJS)', () => { | ||
function foo(state = 0, action) { | ||
return action.type === 'foo' ? 1 : state | ||
} | ||
|
||
function bar(state = 0, action) { | ||
return action.type === 'bar' ? 2 : state | ||
} | ||
|
||
const store = createStore(combineReducers({ foo, bar })) | ||
const observable = Rx.Observable.from(store) | ||
const results = [] | ||
|
||
const sub = observable | ||
.map(state => ({ fromRx: true, ...state })) | ||
.subscribe(state => results.push(state)) | ||
|
||
store.dispatch({ type: 'foo' }) | ||
sub.unsubscribe() | ||
store.dispatch({ type: 'bar' }) | ||
|
||
expect(results).toEqual([ { foo: 0, bar: 0, fromRx: true }, { foo: 1, bar: 0, fromRx: true } ]) | ||
}) | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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