Closed markerikson closed 4 years ago
Could symbol-observable
be added to the store using an enhancer rather than adding it by default?
import $$observable from 'symbol-observable'
export const asObservable = (createStore) => (...args) => {
const store = createStore(...args)
store.observable = function() {
const outerSubscribe = store.subscribe
return {
subscribe(observer) {
if (typeof observer !== 'object' || observer === null) {
throw new TypeError('Expected the observer to be an object.')
}
function observeState() {
const observerAsObserver = observer
if (observerAsObserver.next) {
observerAsObserver.next(getState())
}
}
observeState()
const unsubscribe = outerSubscribe(observeState)
return { unsubscribe }
}
[$$observable]() {
return this // not sure if this should be `this` or `store`
}
}
}
return store
}
const store = createStore(reducer, asObservable)
const store = createStore(reducer, compose(asObservable, applyMiddleware(epicMiddleware)))
The enhancer could be a separate package, included in redux-observable
or as a distinct import (e.g. import { asObservable } from 'redux/observable'
)
This was just a thought when I saw your tweet. It may not be practical or even work at all.
"Could", yes, because a store enhancer returns a new store object and thus gets to completely override any part of the store API (or even add new things that aren't part of the API).
I'm less concerned about the bit of code, and more just wanting to figure out if we can remove the need to depend on symbol-observable
.
Also, from what I can read, Symbol.observable
is meant to be a universal cross-lib indicator that a thing can be treated as an observable, and is part of the notional ES Observables spec rather than the `redux-observable lib.
Yeah, I guess I was suggesting to drop it because it could be added in userland by whoever actually had that requirement.
I only suggested redux-observable as I thought it might make a nice home for a redux to observable bridge.
Some RxJS folks point out that RxJS itself no longer has symbol-observable
as a dependency:
How do we polyfill the Symbol correctly without the dep? As long as that's possible (I don't see why it wouldn't be), then I'm all for removing the dep.
I'm also good with dropping loose-envify. I never quite understood why it was necessary, other than bad architecture in Browserify. That's not really our responsibility to fix.
Per the linked RxJS file, the polyfill boils down to:
export const observable = (() => typeof Symbol === 'function' && Symbol.observable || '@@observable')();
Out of curiosity - what's the argument behind dropping symbol-observable
? Not that I care that much, not using it at all, but the package is really small and self-contained. What is the benefit of inlining its code (and thus spreading duplication in the ecosystem)?
I once had to debug a really nasty timing issue around Symbol polyfilling. It wouldn't probably be completely prevented by having a single package like symbol-observable, but the risk would be radically minimized.
I believe I was debugging an issue on IE11 when our app has stopped to work. And I've managed to investigate the bug into react-dom and react packages not sharing their symbols (like REACT_ELEMENT_TYPE) through module system, but rather through a global scope (which is basically what you are going to do by dropping symbol-observable
package).
My problem was that a Symbol got polyfilled in between react & react-dom loads, so first load (let's say react) did not have a Symbol available globally yet and it has created fallback number types, but latter react-dom had it available so it has used Symbol.for
to create those types. This lead to a mismatch of types - they have to be the same for react & react-dom packages working OK together.
Loading polyfills not at the top of our app was a bug in itself, but is has slipped because the first react-related load was deeply hidden in an utils file referenced by some other code which we have wanted to execute first - as soon as possible, even before polyfills (error reporting related module).
So long story short - if react & react-dom would use a shared package I (or somebody else) wouldn't have to dig into this edge case problem.
I mean, if you redefine what window.Symbol
is midway through execution, there's not really much that can be done to fix that. 🤷♂
Anyways, I think the byte overhead of symbol-observable vs this small polyfill makes it come out in the wash anyways. It's not really a practical issue.
I agree that this was a strange issue on our side, and not something that happens daily - but a centralized package for those React types would actually save hours of debugging in this case. So it's not really that "not much can be done to fix that". I understand though that this is an edge case and not a strong argument in favor of this - just something I've wanted to mention.
As to the byte overhead - it's 90 bytes difference (minified+gzipped on both). So not really something I would personally call a significant gain.
Don't get me wrong - I'm here just to discuss as I'm interested in general reasoning/arguments behind changes like this. Lately, I've noticed people wanting to get rid of all dependencies and I just wonder why that is. Personally I don't like bloated dependencies myself either, but inlining really small helpers just for sake of removing a dep is somewhat against composition for me.
I have commented several times that the JS ecosystem would be well served by trying to do some consolidation and reduce tiny dependencies. There's no reason that a CRA+TS app should end up with 1500 deps downloaded.
In Redux's case specifically, there's no need to add a package dependency just to get this tiny polyfill. I would love to see v5.0 have zero additional dependencies after installation.
I think the modern (2019) approach is to not include polyfills in libraries, but do communicate which polyfills people need to install in order to support certain browsers.
ky is a good example of this:
Ky targets modern browsers and Deno. For older browsers, you will need to transpile and use a
fetch
polyfill. For Node.js, check out Got. For isomorphic needs (like SSR), check outky-universal
.
Sounds good to me.
Still need to get rid of loose-envify
.
Yep. That was GitHub auto-closing it.
Removed in 17f0ea0.
We currently have two external dependencies: https://github.com/zertosh/loose-envify and https://github.com/benlesh/symbol-observable .
loose-envify
appears to be used inpackage.json
, but only as some kind of polyfill / build plugin for Browserify:https://github.com/reduxjs/redux/blob/85024d431ca765c843561be6792edeb7890d5fce/package.json#L102-L106
symbol-observable
is used increateStore
as a polyfill forSymbol.observable
, to allow compatibility for treating the Redux store as an observable:https://github.com/reduxjs/redux/blob/85024d431ca765c843561be6792edeb7890d5fce/src/createStore.ts#L1
https://github.com/reduxjs/redux/blob/85024d431ca765c843561be6792edeb7890d5fce/src/createStore.ts#L331-L333
I would like to strongly consider dropping both of those dependencies if at all possible.
I question whether we need to have any specific support for Browserify in here. If there's a build step that needs to be done, let the end users take care of it. No reason to have an installation dep just for that.
symbol-observable
boils down to this 15-line file for the polyfill plus another 15 lines for figuring out what the global object is.@davidkpiano suggests rolling our own
symbol-observable
if really needed.Finally, we are currently talking about adding
ts-toolbelt
as a dependency for itscompose
function. I would like us to seriously investigate whatever other alternatives are out there to adding a dep, even if it's one that ultimately compiles away.