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

DOMCache saved as json not global var #4

Closed
wants to merge 2 commits into from
Closed

DOMCache saved as json not global var #4

wants to merge 2 commits into from

Conversation

musshush
Copy link
Contributor

This changes save DOM cache as application/json script type rather than using window global variables.

@musshush musshush mentioned this pull request Nov 20, 2014
@musshush
Copy link
Contributor Author

The reason being these changes are made is because a lot of sites don't allow inline javascript because of content security policy.

@StuartHarris
Copy link

Unless I'm misunderstanding something (which is quite possible) this shouldn't be a problem. If the server and the client serialise to differently formatted JSON then React will just correct it. So the server calls JSON.stringify to inject the data in format 'a', then the client calls JSON.parse to read the data in order to render the VDOM tree, during which it will serialise it again in format 'b'. If the two formats are different, then the checksum will be different and React will adjust the innerText of the script tag. But it's the same data so apart from React's warning in dev builds the app behaviour will be identical. Or am i missing something?

@StuartHarris
Copy link

I think a more serious problem, is that now we are round-tripping the data through the DOM, then the shouldComponentUpdate method would need to be removed (or we'd need to only parse on first load) otherwise the data doesn't get updated as you move through client routes. We're probably going to use Isaac's async-cache module instead, so please feel free to close this pull request if you want to :-)
BTW this is the best router out there IMO :-)

@matthewwithanm
Copy link
Owner

@StuartHarris Thanks!

I don't think async-cache is actually solving the same problem domCache is meant to. What we want is a way of guaranteeing that data used on the server is the same as data used by the browser. For example, if your server does an HTTP request for data, the browser shouldn't do that HTTP request because, in the time between the two, the data may have changed (likely resulting in a checksum error). The way we do it is by serializing the data into the DOM on the server and then reading it out on the browser side. So we don't really care about updating the DOM as you move through client routes—just ensuring we get the exact same data in the browser as was used to construct the HTML on the server. The point of domCache is to avoid the checksum errors.

If we take this approach, we'll need to make sure that what we put into the element is always the same. As soon as taking something from innerText becomes part of that equation, things get a little hairy (since we're relying on browsers to all do the same thing for that). I think if we put in some common sense sanitization (.trim() the contents) we should be okay, but I guess we'll have to test.

@ghost ghost closed this by deleting the head repository Sep 8, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants