fetchData() and fetchDataDeferred() behave as they should

[mariocasciaro]

As promised

Please consider you need these patches for it to work:

• https://github.com/mariocasciaro/react-router/tree/fixes (discussed here useRoutes changes the meaning of history.listen remix-run/react-router#2502)
• https://github.com/mariocasciaro/redux-simple-router/tree/fix-loop (already a PR)

[quicksnap]

Thanks for your work on this. This solves all open issues! But, that API issue with react-router still prevents merging =(

[mariocasciaro]

OK, good news, I found a way to avoid the patch in react-router. Now only the patch in redux-simple-router is required!!!

[quicksnap]

Could you elaborate on what you changed?

[mariocasciaro]

Oh sorry, yes, I amended the commit. I just used react-router.match inside history.listen, instead of using useRoutes + listenRoutes. It's essentially another way of getting the route components.

[quicksnap]

Well, great! Thanks for all the time put into this. This is pretty damn close..

[jlongster]

Dooh! I merged the other PRs first and this isn't merge-able anymore. Do you mind rebasing @mariocasciaro ?

[quicksnap]

[mariocasciaro]

Sure, I'll try to find some time tomorrow

[mariocasciaro]

Should be ok to go now

[jlongster]

Thanks!

[jlongster]

I don't think you should be using match browser-side at all. We should be using a history that has been augmented with useRoutes: https://github.com/rackt/react-router/blob/master/docs/API.md#useroutescreatehistory

In fact you pass routes into createHistory but that's not doing anything. You need to also wrap createHistory with useRoutes, and then the listen signature changes: you get (error, nextState) => {} instead, and you can just use nextState to get all the components.

[mariocasciaro]

I agree, but this is mainly a work-around for this: remix-run/react-router#2502
I don't think there is any practical drawback in using match here, but probably I'm missing something?
Besides, useRoutes can be applied only to listen and not to listenBefore, so unless we find another way for pulling the route components in listenBefore (e.g. react-router gets a patch), this is the only code I got working.

[jlongster]

Sorry for being so slow, last week was a holiday. I see a problem with how you are fetching data; I think it can be greatly simplified. Can you take a look?

[mariocasciaro]

I see that you spotted some issues with the logic I implemented, but unfortunately it's the only I got working properly with the current features available in react-router. I found that using createElement is not practical because it's not possible to use setState inside it, and match was the only public API from react-router that worked perfectly without requiring patches.
I agree that the logic could be simplified, but this is what it's working with the current libs. This is just the solution I prepared for my project and thought it could be useful to someone else, so I don't mind closing this PR and re-evaluating the solution once we got all the support we need from react-router.

[jlongster]

Sorry for being slow to respond, work is especially crazy right now. The only drawback is just complexity. If people are going to look to this as an example I want to find the best solution, and using match on the frontend in addition to <Router> is more complex than most people need.

I'd like to just play with it some more, but haven't had time. Definitely don't close this, I may merge and then make some changes after.

[quicksnap]

I agree @jlongster about this being an example of the best solution.

The ongoing issue in react-router is being discussed here now: remix-run/react-router#2614

Might be worth waiting until a resolution.

[jlongster]

Cool thanks for the link @quicksnap. FWIW I have a work week next week so I'm going to be super busy and not very responsive for the next week and a half. This project is a huge thing and I still don't understand all of it, so I'm at least going to be adding more simple examples to redux-simple-router but I'll keep an eye on this as well.

[bdefore]

@jlongster curious if there was ever a resolution for this? i'm seeing infinite loops for all routes except for '/' on version 1.0.1

[bdefore]

more specifically, to trigger the loop:

        try {
          console.log('fetchAllData');
          fetchAllData(
            renderProps.components,
            store.getState, store.dispatch,
            renderProps.location,
            renderProps.params
          ).then(() => {
            console.log('fetchAllData complete', store, renderProps);
            const component = (
              <Provider store={store} key="provider">
                <RoutingContext {...renderProps}/>
              </Provider>
            );

            const status = getStatusFromRoutes(renderProps.routes);
            if (status) {
              res.status(status);
            }
            console.log('sending response');
            res.send('<!doctype html>\n' + ReactDOM.renderToString(<CustomHtml assets={tools.assets()} component={component} store={store} headers={res._headers} />));
            console.log('response sent');
          });
        } catch (err) {
          console.error('DATA FETCHING ERROR:', pretty.render(err));
          res.status(500);
          hydrateOnClient();
        }

and then later, when I call ReactDOM.renderToString(component) it hangs. if i modify the line <RoutingContext {...renderProps}/> to <RoutingContext /> it works, although without the full props.