Skip to content
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

Redux Integration #4668

Merged
merged 8 commits into from
Mar 10, 2017
Merged

Redux Integration #4668

merged 8 commits into from
Mar 10, 2017

Conversation

timdorr
Copy link
Member

@timdorr timdorr commented Mar 9, 2017

A wild react-router-redux appears!

This is mostly copied from the current project, as it all works as-is. However, it drops the whole synchronizing-location-when-time-traveling thing. It was complex and I don't get the sense it was that popular of a use case. The important part was getting location into Redux's state, so you could easily get to it with connect(). If people want it that badly, we can bring it back and update for history 4.x (it shouldn't be a ton of work to do so).

It adds a <ConnectedRouter> type of router that takes in a history prop and can read the store from <Provider> (or a store prop, of course).

Some other miscellany:

  • Test suite is built with Jest. I'm a fan.
  • UMD build is with Rollup instead of webpack. We were getting a 50% size reduction by switching Redux to it, so it seems worthwhile.
  • I switched Lerna to independent mode, since react-router-redux is already at 4.0.8. Hopefully this won't cause huge headaches...

I'll add some comments in where I've got some sticking points before this should get merged.

@@ -1,6 +1,6 @@
{
"lerna": "2.0.0-beta.32",
"version": "4.0.0-beta.8",
"version": "independent",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done as a separate commit (d296fa9) so we can revert easily.

class ConnectedRouter extends Component {
static propTypes = {
store: PropTypes.object,
history: PropTypes.object,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure if we wanted to provide a reasonable default here (create a browser history if none provided) or set up prefab <Connected(Browser/Hash/Memory)Router> exports.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My personal opinion as an end-user is to serve prefab exports, this avoids me on my end having to put a reference to the history module and init it and pass it in. It also means I dont need a dependency in my package to history (I know I could ignore it, but then some of my tools throw warnings) -- and personally you guys are better equipped to keep those dependencies updated along side react-router.

Just my 2¢

fs.readFileSync('umd/react-router-redux.min.js')
)

console.log('\ngzipped, the UMD build is %s', prettyBytes(size))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current size is 1.21 kB

@timdorr timdorr removed the bug label Mar 9, 2017
@pshrmn
Copy link
Contributor

pshrmn commented Mar 9, 2017

As a casual observer, can you verify if I understand this correctly: the main purpose of the <ConnectedRouter> is to dispatch the current location to the store whenever the <ConnectedRouter> re-renders?

@timdorr
Copy link
Member Author

timdorr commented Mar 9, 2017

Yeah, the location gets dispatched as a LOCATION_CHANGE action to the store whenever that <Route> re-renders, which should be whenever the <Router> gets a location update from history.

@pshrmn
Copy link
Contributor

pshrmn commented Mar 9, 2017

Is there any particular reason that has to be done by the router? Would a <SynchronizedLocation> that is rendered inside of one of the pre-existing routers work just as well?

<Provider store={store}>
  <BrowserRouter>
    <SynchronizedLocation store={store}>
      ...
    </SynchronizedLocation>
  </BrowserRouter>
</Provider>

@timdorr
Copy link
Member Author

timdorr commented Mar 9, 2017

Hmm, that might end up being better.

One thing people have constantly asked about is getting the router params into Redux. That wasn't possible before because it only integrated with history, not the router directly. Now the issue is you can have multiple matching Routes all throughout the component tree, so params are relative to that route and what's underneath it.

So, what could be cool here, is to actually do a <ConnectedSwitch>. Since that gets you down to a single route (and therefore match.params), you could finally get those pesky params into state.

@ryanflorence
Copy link
Member

ryanflorence commented Mar 10, 2017

Yeah, if people want params we could probably do it with <ConnectedRoute|Switch> Looks great Tim!

@ryanflorence
Copy link
Member

ryanflorence commented Mar 10, 2017

Can people dispatch to navigate? That's another thing we get all the time. What I'm after in this package is:

  • Router location in the store (to trigger deep updates, have available in connect)
  • Dispatch actions to navigate (people always ask for it, will require middleware I think)
  • Devtools to work across route changes (the url updating in the browser isn't important, but the devtools should still work)

Bonus would be matches available in the store too, though I'm not sure what that even looks like in the store, I imagine it'd be deeply nested tree to mirror the UI hierarchy that created it, and it would be weird to connect to the right match since mapStateToProps is a user-land thing.

@ryanflorence
Copy link
Member

As for jest/rollup, I'm into it, but @mjackson has more say on that stuff than me.

@pshrmn
Copy link
Contributor

pshrmn commented Mar 10, 2017

The biggest blocker from using Jest is that it doesn't support browser compatibilty testing, correct? I know that I've seen a few test failures caused by mobile Safari.

Keeping match/param data in the store seems like it would be a nightmare to handle. You would probably need to add route keys to store/fetch the correct data.

@timdorr
Copy link
Member Author

timdorr commented Mar 10, 2017

Yeah, Jest is node-only. But I'm only using a memory history for testing, so I don't need a DOM (real or fake).

You would probably need to add route keys to store/fetch the correct data.

Just discussed that with Ryan. We could do something like this:

<ConnectedRoute path="/foo" matchStoreKey="foo" component={Foo} />
<ConnectedSwitch matchStoreKey="baz">
  <Route path="/foo" component={Foo}/>
  <Route path="/bar" component={Bar}/>
</ConnectedSwitch>

That would put it into a known place in the store so you can reference each one.

Also, we can set it to listen to the location from the store instead of from context, which means time travel should be a little easier. I don't know if that totally solves the problem of time travel, but it's a reasonable start.

I'd also love to get some of the bits of @supasate's connected-react-router added in eventually. This is a first pass and he's got a head start on me, so I know there's some good bits of code gold to pull from. Plus, merging our efforts will be beneficial to everyone.

@timdorr
Copy link
Member Author

timdorr commented Mar 10, 2017

BTW, in talking with @ryanflorence, we decided to just merge this in as-is for now and build on it with other PRs. If it's totally boned, that's fine because it's still an alpha.

And I've reached out to @supasate and we'll see about merging in some of the connected-react-router goodness.

@timdorr timdorr merged commit 85fec3c into v4 Mar 10, 2017
@timdorr timdorr deleted the dux-again branch March 10, 2017 19:04
@supasate
Copy link
Contributor

Thanks @timdorr. I'll join the force!

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants