-
Notifications
You must be signed in to change notification settings - Fork 33
Updated react-redux bindings to v6 and updated examples #103
Conversation
@@ -1,22 +1,26 @@ | |||
import React from 'react' | |||
import { Route, Switch } from 'react-router' | |||
import { SubspaceProvider } from 'react-redux-subspace' | |||
|
|||
// work around for lerna/npm link issue for local development | |||
import { ReactReduxContext } from 'react-redux' |
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.
You'll see this a lot in the review. When lerna
links the packages using npm link
they end up with different instances of react-redux
so the React.createContext(...)
calls create a different context so react-redux-subspace
cannot access the store from the examples Provider
. This follows the "BYO contextconcept from
react-redux` but provides the actual context instance from the example's version.
This works, but it's a bit gross and not really how you would use in a real app, which makes having it in all the examples a bit annoying.
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.
This is one annoying thing about reacts new context api.
Could we have a post install script that removes the nested react-redux packages in node_modules so they all resolve to the same one?
At least then the fix is hidden away in the package.json and not in the example code.
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.
Actually, that would break the build of the actual package... Ignore that.
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.
The issue with that is the file is not in the same directory tree, so the node package resolution does not find the package at all. I have done a trick in the past to npm link
the troublesome package back to the other's as a post-installation task, but I found it to be slow and required a bit of hacking about to get working. At least this is actual functionality offered by both react-redux
and now redux-subspace
, it just sucks a bit that it's the defacto example.
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.
At least it won't break if someone uses it in production code (it'll be the same as the default value)
Maybe adjust the comment a bit to make it more obvious?
"// Note: please don't copy this line (and associated prop usage) to your project, you are unlikely to need to do this for standard subspace usage.
// This is caused by lerna using npm link..."
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.
lol at autocorrect, but I like the idea.
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.
That'll teach me for reviewing on my phone.
"prop-types": "^15.6.0", | ||
"@types/react": "^16.8.3", | ||
"hoist-non-react-statics": "^3.3.0", | ||
"memoize-one": "^5.0.0", |
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.
new dependency
"recompose": "^0.30.0", | ||
"redux-subspace": "^3.0.1" | ||
}, | ||
"peerDependencies": { | ||
"react": "^15.1 || ^16.0", | ||
"react-redux": "^5.0.0", | ||
"react-redux": "^5.0.0 || ^6.0.0", |
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.
current implementation should be backwards compatible, but it has complicated the internals. We could do a clean cut over and do a major bump instead. I'm just worried about how we currently distribute subspace
and dynostore
through our bundles, not having backwards compatibility will make upgrading harder.
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.
Yeah we will definately want to have this version supporting both.
Even if we release the major bump soon after just to cleanup and prepare for a "no legacy context" future, well probably be running this version for a while until we can port all the micro-frontends over.
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.
Talking internal to ioof now. Because of the new context api and having to be the same reference, we're going to externalize subspace (and dynostore?), Upgrade all the micro-frontends so they are using the same version from the parent, then upgrade react-redux.
We actually won't need to support both contexts at once? If we first externalize the old version and then cut over to the new.
We may still want to have this version for others to migrate with.
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.
Because the context we use comes from react-redux
which is already externalised, we don't have to externalise subspace or dynostore for this change...
But I really, really think we should so that future changes like this are easier. Anything where there is a provider in one bundle and a consumer in another should, in my opinion, be externalised.
SubspaceProvider.childContextTypes = { | ||
store: PropTypes.object | ||
} | ||
const storeAccess = compose(contextApi, legacyContext) |
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.
2 way to access the store, both pass the store as a prop to the underlying SubspaceProvider
only if they find it in their respective methods. Technically with this change, a store can be passed directly as a prop if there is no Provider
(or SubspaceProvider
) higher in the tree.
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.
Providing your own store could be useful for testing?
} | ||
|
||
class SubspaceProvider extends React.Component { | ||
makeSubspace = memoizeOne((mapState, namespace, subspaceDecorator, store) => { |
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.
We actually get a perf boost from this change as we are only going to recalculate the subspace if the params to create it change. Previously we would recalculate it any time getChildContext
was called (which I believe is on every render). The improvement is somewhat negligible because of the PureComponent
used before, which has been removed now.
const subspace = this.makeSubspace(mapState, namespace, subspaceDecorator, store) | ||
return ( | ||
<Provider key={subspace.namespace} store={subspace}> | ||
<SubspaceLegacyContextOutput subspace={subspace}> |
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.
This will cause the subspace to always be put into legacy context for and downstream SubspaceProviders
or dynostore
dynamic
components using older versions to access.
I'm not a big fan of this, but I can't see another way that still has backwards compatibility.
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.
Definately a necessary evil.
|
||
export default SubspaceProvider | ||
export default storeAccess(SubspaceProvider) |
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.
because this maintains legacy context support, subspacing a single component using
<SubspaceProvider mapState={(state) => state.example} namespace='example'>
<Component />
</SubspaceComponent>
ends up with a component heirarchy of
<SubspaceContextApi>
<ReactReduxContext.Consumer>
<SubspaceLegacyContextInput>
<SubspaceProvider>
<Provider>
<ReactReduxContext.Provider>
<SubspaceLegacyContextOutput>
<Component />
</SubspaceLegacyContextOutput>
</ReactReduxContext.Provider>
</Provider>
</SubspaceComponent>
</SubspaceLegacyContextInput>
</ReactReduxContext.Consumer>
</SubspaceContextApi>
If you add in the subspaced
HOC, the top level Provider
and the layers for the connect
HOC of the component, this starts to look a bit crazy. In fairness, the legacy context support is only 2 of the 7 layers, so not having it only makes the problem a little bit better.
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.
Anyone running an app complex enough to need subspace likely has worse demons in their component tree.
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.
We may want to have a further chat about backwards compatibility and how long we want to support that for.
But I approve of this in concept and approach.
How long will I wait for the new version? Will you accept the pull request? Looking forward to the new version. @jpeyper |
Hi @erdembas, In all honesty, I've been busy at work and as we aren't in a position to actually update react-redux to v6 ourselves, this has taken a bit of a backseat in my priorities. We still need to:
I'll try to get this closed off soon. Sorry for the inconvenience. |
Hi @mpeyper , I will be very pleased if it is closed soon. I'm waiting :) |
As another bit of info, V7 is already out for Redux |
To work around this, i've created two components that let you shim between redux-subspace (in it's current form) and redux 7.x:
|
I know people have been waiting for this, and I'm sorry it's dragging on. I plan on revisiting this tomorrow and getting it updated to the Redux 7.1 release. I also plan on removing the multi version support. It's going to be v6+ (i.e. context api) compatible only. |
Closing this now that #117 is (finally) up. |
Support for react-redux v6.x.x using the new context API.
More discussion in the code.