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

Fix HistoryLocation in React v0.13 Example #67

Merged
merged 9 commits into from
Aug 20, 2015
Merged

Conversation

ericclemmons
Copy link
Owner

Hi!

Using HistoryLocation, instead of RefreshLocation as used in the examples, I'm only able to navigate via react-router (both Link component and transitionTo('')) once pr. page load. I've tried to fiddle around with both the source for react-router and react-resolver, but without success. The URL changes, but the

I've reduced my issue down to this repo, which can be run with npm install and npm run watch. If you uncomment 11th line and comment the 10th line in src/client.js, you'll notice that routing works, but obviously serverside rendering does not. This leads me to think this could be a Resolver problem.

Anyone having the same issue? Have I done something wrong? I've tried to implement Resolver as per the examples as close as possible.

@ericclemmons
Copy link
Owner

@ostrgard This seems to be a bug with React Router v0.13, whereas it works fine in React Router v0.14.

The crazy part is, router.run(function() { // THIS SHOULD BE CALLED ON TRANSITION }) never runs.

Thanks for putting together this demo! What I'm afraid of is there being an issue with context, which is well-known in React v0.13, but I hope there's a solution.

@ostrgard
Copy link
Author

Thanks for the answer, Eric.

I've just noticed a mistake I made. I put the __REACT_RESOLVER_PAYLOAD__ after loading the script. So I had no initial data. Doh. It does not entirely solve my problem though.

Can you explain Resolvers render function to me?

static render = function(render, node) {
    const initialData = window.__REACT_RESOLVER_PAYLOAD__;

    // Server-rendered output, but missing payload
    if (!initialData && node.innerHTML) {
      return Resolver.resolve(render).then(({ Resolved }) => {
        // @TODO - Use react-dom.render
        React.render(<Resolved />, node);
      });
    }

    // @TODO - Use react-dom.render
    React.render((
      <Resolver data={initialData}>
        {render}
      </Resolver>
    ), node);

    delete window.__REACT_RESOLVER_PAYLOAD__;
  }

If I comment out the last line, my routing works. Do you mean to execute the if statement block on second render? Maybe you meant to do !initialData && !node.innerHTML to make sure the node is actually empty.

@ericclemmons
Copy link
Owner

Sure! (This will end up going in the docs to avoid confusion)

  • Resolver.render(() => <Foo />, document.getElementById("app")) on an empty <div id="app"></div> node will render just like React.render would - completely async.
  • If the <div id="app"> already has content, the Resolver will render the entire tree in memory and then mount on <div id="app">. This prevents clearing the DOM while data is loaded on the client-side.
  • Finally, the ideal scenario is having __REACT_RESOLVER_PAYLOAD defined from the server in which Resolver.render() is a synchronous operation since all of the data already exists.

Are you saying that by commenting out delete window.__REACT... it worked?

@ericclemmons
Copy link
Owner

All I'd need to see is the order & how you're outputting the payload + script.

Notice how in the React v0.13 Example I use <script defer> to have to script on after DOMReady.

Perhaps I should remove that since many aren't aware of how that works?

@ostrgard
Copy link
Author

I'm sure it worked, yes. Try the repo i created (make sure you pull the latest, I've positioned the payload, so it's present for the script). Go into your build file in node_modules and comment it out. I've located it to when this renders...

Resolver.resolve(render).then(({ Resolved }) => {
        // @TODO - Use react-dom.render
        React.render(<Resolved />, node);
});

... it stops running the render function for the rest of the app lifetime. You wrote a comment to this if statement: "Server-rendered output, but missing payload". But it runs the first time I navigate – we did delete the payload and the node is populated. Commenting out delete window.__REACT_RESOLVER_PAYLOAD__; just resulted in this never running, which is why it worked.

@ericclemmons
Copy link
Owner

Great debugging! I really appreciate it!

I'll dig into this then & push out a fix...

@ericclemmons ericclemmons self-assigned this Aug 13, 2015
@ostrgard
Copy link
Author

Sounds good! Really nice work you've done so far! Please ask if you need more info!

@ericclemmons ericclemmons mentioned this pull request Aug 13, 2015
@ericclemmons ericclemmons changed the title React Router, Resolver and HistoryLocation Fix HistoryLocation in React v0.13 Example Aug 13, 2015
@ericclemmons
Copy link
Owner

This also seems related to #69, which I'm taking care of in this issue...

@ericclemmons
Copy link
Owner

I was able to isolate the bug to specifically client.js:

// Works with HistoryLocation
React.render(<Root />, document.getElementById("app"));

// HistoryLocation does not update past the first or second route
Resolver.render(() => <Root />, document.getElementById("app"));

I'll dig in, but I'd wager it's because after Resolve.render "finishes", the resolved data is never cleared out...

static render = function(render, node) {
const initialData = window.__REACT_RESOLVER_PAYLOAD__;

static render = function(render, node, data = window[PAYLOAD]) {
Copy link
Owner Author

Choose a reason for hiding this comment

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

This was the crux of the solution: If the DOM has content but the payload is missing, _fetch the payload & call Resolver.render with it :)

Sometimes the best answer for recursion is recursion!

@ericclemmons
Copy link
Owner

This is nearly fixed, but I've also cleaned up the examples a bit as well.

@ericclemmons
Copy link
Owner

Confirmed working! I'll merge & patch release this shortly...

@ericclemmons
Copy link
Owner

So sleepy...I'll do this tomorrow.

@ostrgard
Copy link
Author

Happy you found a solution. Sleep tight!

@ostrgard ostrgard closed this Aug 18, 2015
@ericclemmons
Copy link
Owner

Uh... shoot. You shouldn't have closed this.

@ericclemmons ericclemmons reopened this Aug 18, 2015
@ericclemmons ericclemmons changed the title Fix HistoryLocation in React v0.13 Example [WIP] Fix HistoryLocation in React v0.13 Example Aug 18, 2015
@ostrgard
Copy link
Author

Please forgive my lack of Github skills.

@ericclemmons ericclemmons changed the title [WIP] Fix HistoryLocation in React v0.13 Example Fix HistoryLocation in React v0.13 Example Aug 20, 2015
ericclemmons added a commit that referenced this pull request Aug 20, 2015
Fix HistoryLocation in React v0.13 Example
@ericclemmons ericclemmons merged commit 2a77eab into master Aug 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants