ericclemmons / react-resolver

Async rendering & data-fetching for universal React applications.
https://ericclemmons.github.io/react-resolver
Other
1.65k stars 52 forks source link

[HOLD] React Router v1 messes up with missing payload #69

Closed alexbyk closed 7 years ago

alexbyk commented 9 years ago

Hello. I'm trying to make a simple project with react-resolver + react-router. But it doesn't work. I've placed it here: https://github.com/alexbyk/react-resolver-example

Could you pls take a look... what am I missing? The simple click action causes an error (without Resolver works fine): Uncaught TypeError: Cannot read property 'firstChild' of undefined && TypeError: deepestAncestor is undefined

ericclemmons commented 9 years ago

@alexbyk I can take a look at this soon. Under the weather today :(

The React v0.14 example uses the latest React Router as well and I hope helps in the meantime.

alexbyk commented 9 years ago

I figured out where the problem is. React-resolver doesn't work if the page wasn't generated by the server. Looks like a bug?

ericclemmons commented 9 years ago

@alexbyk That's bizarre. I have it running in both scenarios.

I'll confirm...

alexbyk commented 9 years ago

I also found a workaround: https://github.com/alexbyk/react-resolver-example/commit/bd0806670235739b038a6723924e049c6b8ffb03 so React.resolver doesn't work without !!window.__REACT_RESOLVER_PAYLOAD__

ericclemmons commented 9 years ago

Oh, interesting! Man, thanks so much for this! I'm fixing all of these bugs this time around and you're update makes it possible :)

ericclemmons commented 9 years ago

Should should be resolved via v2.0.3 & #67.

alexbyk commented 9 years ago

:( still doesn't work without https://github.com/alexbyk/react-resolver-example/commit/bd0806670235739b038a6723924e049c6b8ffb03

ericclemmons commented 9 years ago

@alexbyk I'm not sure what to say. By all of my tests, it works in the React v0.13 example:

proof

Can you clarify your exact setup?

alexbyk commented 9 years ago

Hi, @ericclemmons Your examples work fine (and they were working in 2.0.2 too) I just tried to run this project https://github.com/alexbyk/react-resolver-example (after your updates) and it is still not working without window.__REACT_RESOLVER_PAYLOAD__ = {}. While adding window.__REACT_RESOLVER_PAYLOAD__ = {} solves the problem, this isn't big deal, but it's still a bug.

Maybe I'll try to find it myself and if I find, I'll send a pull request

ericclemmons commented 9 years ago

Wow, @alexbyk, I just ran your example & sure enough, it works with:

diff --git a/index.js b/index.js
index e3d9e0c..7522bb1 100644
--- a/index.js
+++ b/index.js
@@ -51,4 +51,4 @@ const router =
   </Router>;

 //React.render( router, document.getElementById("root"));
-Resolver.render( () => router, document.getElementById("root"));
+Resolver.render( () => router, document.getElementById("root"), {});

I'll fix it. Thanks again for the awesome demo :)

ericclemmons commented 9 years ago

Yep, this only affects React Router v1!

ericclemmons commented 9 years ago

This is very reminiscent of:

http://stackoverflow.com/questions/27153166/typeerror-when-using-react-cannot-read-property-firstchild-of-undefined

Peculiar, nonetheless. I can't actually explain why this would be a problem, since it only seems to happen with React Router v1.

ericclemmons commented 9 years ago

@alexbyk Ok, I think the only solution is to actually remove this functionality:

https://github.com/ericclemmons/react-resolver/blob/a1704ee3f355ea1fb4264b4e844e79dd88480f81/src/Resolver.js#L34-L40

Here, I allow users to do SSR but omit the payload in the event that the client may already have much of the data cached or fetching it is more efficient. (In my scenario, the API requests are highly normalized at 1/8th the size that is denormalized in the app)

This means that, for this scenario, much of the app will likely be wiped out why the data gets resolved.

I wish I could find a solution around this...

ericclemmons commented 9 years ago

Yea, it's almost as if the options are:

This would remove the ability to:

ericclemmons commented 9 years ago

@alexbyk Can you test out this branch? In my testing, React Router v1 works as expected, but you're my guinea pig here :)

alexbyk commented 9 years ago

Hey, @ericclemmons I can confirm that the bug is fixed in the "69-..." branch. Thanks!

ericclemmons commented 9 years ago

Haha! I converted this to a PR, so I gotta open it up again :)

I'll take care of releasing this. Thanks for confirming!

ericclemmons commented 9 years ago

Ok, I'm less bullish on merging this.

Going to wait for React Router v1 to be released & see if it's still an issue.

ericclemmons commented 7 years ago

Closing due to inactivity.