Remove enableFilterEmptyStringAttributesDOM#31765
Remove enableFilterEmptyStringAttributesDOM#31765rickhanlonii merged 3 commits intofacebook:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
CLA Signed
React Core Team
sebmarkbage
left a comment
sebmarkbage
left a comment
There was a problem hiding this comment.
I noticed a bug with this today. Empty href string doesn't hydrate on the client since the attribute is omitted from the server. Causing a hydration error (in addition to the warning).
|
Comparing: 3ad17ecd313a8e53b339adf8052e35b3d73f8c62...fda9cd776e90bfd6e91e133b4f9d54ee6e3c1ae4 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
| propKey, | ||
| propKey, | ||
| ); | ||
| } |
There was a problem hiding this comment.
I think we should probably skip hydrateSanitizedAttribute below if we already warned above.
I.e. call continue in the branch above.
Otherwise we end up both logging the error and with a hydration error, because the server will have omitted this attribute.
The hydration error is also not correct because it's not what the client would've done.
There was a problem hiding this comment.
Do you mean only in the DEV branch above? That seems weird to differ in behavior?
There was a problem hiding this comment.
I think what you're saying is that this would be how to fix the bug you found, so i'll merge with the existing behavior and we can fix the bug in a follow up
There was a problem hiding this comment.
Well the DEV branch is defensive. This whole function is inside a DEV branch so it's not divergent.
Base: #31765 Landed everywhere
Base: facebook#31765 Landed everywhere DiffTrain build for [1520802](facebook@1520802)
Base: facebook#31765 Landed everywhere DiffTrain build for [1520802](facebook@1520802)
I think this is the suggested change from #31765 (comment) But no tests fail and I'm not sure how to test it? Seems sus. Also seems like the `removeAttribute` here should be changed? https://github.com/facebook/react/blob/9d9f12f2699a049777fa88914306ad4de9e2b74d/packages/react-dom-bindings/src/client/ReactDOMComponent.js#L400-L427
I think this is the suggested change from #31765 (comment) But no tests fail and I'm not sure how to test it? Seems sus. Also seems like the `removeAttribute` here should be changed? https://github.com/facebook/react/blob/9d9f12f2699a049777fa88914306ad4de9e2b74d/packages/react-dom-bindings/src/client/ReactDOMComponent.js#L400-L427 DiffTrain build for [bf883be](bf883be)
I think this is the suggested change from #31765 (comment) But no tests fail and I'm not sure how to test it? Seems sus. Also seems like the `removeAttribute` here should be changed? https://github.com/facebook/react/blob/9d9f12f2699a049777fa88914306ad4de9e2b74d/packages/react-dom-bindings/src/client/ReactDOMComponent.js#L400-L427 DiffTrain build for [bf883be](bf883be)
I think this is the suggested change from facebook#31765 (comment) But no tests fail and I'm not sure how to test it? Seems sus. Also seems like the `removeAttribute` here should be changed? https://github.com/facebook/react/blob/9d9f12f2699a049777fa88914306ad4de9e2b74d/packages/react-dom-bindings/src/client/ReactDOMComponent.js#L400-L427 DiffTrain build for [bf883be](facebook@bf883be)
I think this is the suggested change from facebook#31765 (comment) But no tests fail and I'm not sure how to test it? Seems sus. Also seems like the `removeAttribute` here should be changed? https://github.com/facebook/react/blob/9d9f12f2699a049777fa88914306ad4de9e2b74d/packages/react-dom-bindings/src/client/ReactDOMComponent.js#L400-L427 DiffTrain build for [bf883be](facebook@bf883be)
5 participants
Base off #31764
This has landed everywhere