-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Isomorphic rendering sporadically renders attributes in a different order #6451
Comments
This is most likely due to v8 using the incorrect order in |
@mridgway No such luck... Ran I'm using chrome Version 49.0.2623.87 (64-bit), and updating chrome now to see if that makes a difference. |
If you have |
@zpao Thank you sir...
did the trick! |
@zpao Hmm, wasn't this one of the original reasons for the polyfill? I seem to remember this exact discussion. |
It does sound familiar but I didn't think that was part of the original discussion. I recall that we were just going to use Object.assign directly and then there was an issue with the spec (it would throw when getting FWIW, I think the problem that's being exposed here could have been encountered before, especially if you ever used spread in JSX (or even regular object spread) and use Babel. Both of these get compiled to using Object.assign (essentially). The difference though is probably frequency, instead of just hitting it for those cases, it's happening for normal use of props. I'm not sure the best way to handle this. I really want us to be able to use native code and not end up relying on our own dated polyfill. See #6376 for that discussion. We assumed it was perfectly safe, particularly due to the fact that we effectively have been using native Object.assign for facebook.com for at least a year (we did some module replacement internally). We don't do much server rendering so did not properly consider this case (and honestly completely forgot that v8 still has this bug) |
Seems like this would be a problem Babel spread too. Typically this is the kind of issue a polyfill should test for before assuming compatibility of the native impl. |
I wish we checked for validity by walking the DOM rather than calculating a checksum for markup. It's more about us not being spec-compatible, rather than a V8 bug. |
No, V8 is not spec compliant. The enumeration order is defined by spec and fixed in newer V8s. There are other APIs like Intl number formatting that does allow for different output. That's a separate issue and even if we walked the DOM that wouldn't automatically solve that issue. |
Ah, ok, they fixed the spec :). We're depending on previously undefined behavior. But the behavior is still undefined if we're targeting anything below ES6, so one could argue that we should solve this within React anyway. |
Only pasting the text from the other issue here: I'm developing an universal app with React, and after updating to v15.0.1, I started getting this error:
It seems to be some bug on V8 Tried to change a bunch of stuff (e.g: force both server and client to use a polyfill, or using Babel transform object-assign plugin), but nothing solved the problem. Then, I uninstalled React 15.0.1 and installed 0.14.8 again, and the problem was gone. Btw, it's pretty simple to simulate the issue: Code ReactDOM.render(
<input name="my-input" className="my-class" type="number" pattern="[0-9]*" />, document.querySelector('#main')
);
console.log(document.querySelector('#main').innerHTML)
console.log(ReactDOMServer.renderToString(
<input name="my-input" className="my-class" type="number" pattern="[0-9]*" />
)); React 15 not working React 0.14.8 working as expected |
Yes, V8 has a bug in their Object.assign implementation. I've heard the bug might have been fixed but that's really unlikely to end up in Node v4.x || 5.x. |
Diving into the code, it seems that it has nothing to do with the V8 If you open jsBin in Firefox, and paste the code above, React v0.14 will work as expected, while React v15.0 will mess the attributes. It seems that the problem is because until v15, React created the elements using I can see two ways of solving this problem: |
^ cc @spicyj |
Ideally that is what we would do but I think you are definitely right and it would be expensive. Server rendering is already relatively expensive / slow, so you might lose the advantages you would otherwise get. |
When reviving server-rendered markup, we generate markup on the client using the exact same codepath and generate the checksum based on that – so any changes to the createElement mode won't affect checksums and are unrelated. |
It sounds like there are two related bugs in V8 that mess with the property enumeration order (thanks @mridgway for digging on these): https://bugs.chromium.org/p/v8/issues/detail?id=4118 Sounds like the best solution is to send a PR to https://github.com/sindresorhus/object-assign that tests for these cases and doesn't use Object.assign if so. |
@spicyj The only thing that I can't understand is why with React 0.14.8, everything works smoothly, and when upgrading to React 15.0.1, the isomorphic rendering stops working. It doesn't seem to be something related to |
@buzinas In React 0.14.8, we had an internal polyfill for |
@FuzzySockets @jimfb I'm probably doing something wrong, then. Because I'm forcing the Object.assign polyfill, but my app keep warning about the checksum :( |
@buzinas In your entry, add:
|
#6451 (comment) works because it deletes the native implementation first, so the polyfill doesn’t use it. |
Now everything makes sense :P PS: It works! 💃 |
Not saying it’s an official recommendation though. Ideally |
Incorrect property ordering is causing us some problems downstream at facebook/react#6451.
I just released a new version of object-assign (4.1.0) that includes a feature test for these V8 bugs. Can someone reinstall and try with the new version and verify that this is fixed? |
@spicyj seems to have resolved the issue for me! was getting the mismatched checksum very consistently (~80%) and haven't seen it once since re-installing all modules. It appears I can even remove |
Sounds great. I'll close this; hopefully it works for everyone else too. |
|
I'm using [email protected] and still getting this. It only started once I upgraded to react 15 |
Please provide a project reproducing the issue. Also please make sure you don’t have an older |
Yeah I did check the tree because that was my first thought, I'll see if I can find some time put together a sample project on Monday. |
On digging further it turns out [email protected] did solve it for all other pages. I was running into this and didn't realize it was a different issue. Please disregard the noise. |
Apologies for the naive question, but where in my project does this object.assign switcheroo need to occur? Lot of comments saying I need to do it, but I haven't a clue what file to do it in. (I may have other issues besides just the placement. Typescript seems angry at the period in Object.assign when I try to import it; I get "= expected". I'm also throwing on having a bad path to object-assign, but I think I can probably overcome those problems) |
Assuming you're using npm – if you |
I'm hitting something similar, where using an object spread is reordering my props. In particular, I have the following symptoms—with const p1 = this.props;
const p2 = {...p1};
const s1 = Object.keys(p1).join(", ");
const s2 = Object.keys(p2).join(", ");
if (s1 !== s2) {
console.log(s1);
console.log(s2);
console.log(Object.assign === require("object-assign"));
console.log();
} I get
which really seems like a bug to me. Frustratingly, I can reproduce this 100% within this component but can't seem to reproduce it elsewhere. I do note that the Babelified code uses I'm on [email protected] and react{,-dom}@15.3.1, [email protected], [email protected]. Does this point toward a babel issue? [email protected], [email protected]. Adding Grateful for any advice. |
@wchargin would you be able to share a small test case reproducing this? |
@aweary Here's the smallest I can manage: https://github.com/wchargin/object-spread-repro-case. The only dependencies are Babel, React, and Webpack. As noted at the top of You should see the following: $ git clone [email protected]:wchargin/object-spread-repro-case.git
Cloning into 'object-spread-repro-case'...
remote: Counting objects: 14, done.
remote: Compressing objects: 100% (10/10), done.
remote: Total 14 (delta 3), reused 14 (delta 3), pack-reused 0
Receiving objects: 100% (14/14), done.
Resolving deltas: 100% (3/3), done.
Checking connectivity... done.
$ cd object-spread-repro-case/
$ [ -d node_modules ] && rm -rf node_modules
$ npm install >/dev/null 2>/dev/null
$ npm run build
> @ build /home/wchargin/git/object-spread-repro-case
> rm -rf dist && webpack
Hash: 1fd7ef5334c0d28ceca1
Version: webpack 1.13.2
Time: 1312ms
Asset Size Chunks Chunk Names
bundle.js 706 kB 0 [emitted] main
+ 167 hidden modules
$ npm run demo
> @ demo /home/wchargin/git/object-spread-repro-case
> node demo.js
First render...
className, onClick, aria-pressed, aria-role, children
className, onClick, aria-pressed, aria-role, children
className, onClick, aria-pressed, aria-role, children
className, onClick, aria-pressed, aria-role, children
Second render...
className, onClick, aria-pressed, aria-role, children
onClick, children, aria-pressed, aria-role, className
^^^ Reproduced the bug!
className, onClick, aria-pressed, aria-role, children
onClick, children, aria-pressed, aria-role, className
^^^ Reproduced the bug!
$ node --version && npm --version
v4.2.4
3.10.6
$ npm ls | grep object-assign
$ npm ls | grep object-assign
│ └── [email protected]
$ |
@wchargin Most likely you need to move the |
Yeah, best to include |
Thanks, @mridgway @spicyj. I'd considered that that might be the problem, and tried to do this as // index.js
const isServerRendering = typeof document === "undefined";
if (isServerRendering) {
Object.assign = null;
Object.assign = require("object-assign");
}
import initializeClient from './client';
if (!isServerRendering) {
initializeClient();
}
export {default} from './server'; but this didn't work. What I failed to realize was that Babel hoists imports including const isServerRendering = typeof document === "undefined";
if (isServerRendering) {
Object.assign = null;
Object.assign = require("object-assign");
}
if (!isServerRendering) {
require("./client").default();
}
module.exports = {
default: require("./server").default,
}; All appears to be peaceful now. TL;DR for people passing by later: read the generated code to make sure your polyfill really is happening when you think it is. |
Got it. Nice sleuthing. |
Summary: I was running into facebook/react#6451 even though I'm on React 15.3.1 and `npm ls | grep object-assign` shows only `[email protected]`. A bit of debugging shows that the polyfill wasn't being applied early enough in the node bundle that's used for server-side rendering; we have to be sure to include it before any React components are used, so we might as well make it the very first thing! Test Plan: Load localhost:8080/skills and see that there aren't any SSR errors.
Summary: I was running into facebook/react#6451 even though I'm on React 15.3.1 and `npm ls | grep object-assign` shows only `[email protected]`. A bit of debugging shows that the polyfill wasn't being applied early enough in the node bundle that's used for server-side rendering; we have to be sure to include it before any React components are used, so we might as well make it the very first thing! Test Plan: Load localhost:8080/skills and see that there aren't any SSR errors.
Server side and client side should use match |
I'm using react 15.0.0
The text was updated successfully, but these errors were encountered: