Split markup generation from DOM property management #10197
Conversation
|
This exposed a weird warning: Probably a validation bug. |
|
Should be ready now. |
| @@ -352,6 +352,11 @@ describe('ReactDOMServerIntegration', () => { | |||
| const e = await render(<a href={false} />); | |||
| expect(e.getAttribute('href')).toBe('false'); | |||
| }); | |||
|
|
|||
| itRenders('no string prop with null value', async render => { | |||
There was a problem hiding this comment.
I only added null cases because others were already handled by existing tests.
| ); | ||
|
|
||
| itRenders('numeric property with zero value', async render => { | ||
| const e = await render(<ol start={0} />); |
There was a problem hiding this comment.
This matters because of #2113. 0 shouldn't be removed in this case.
| padding: '4px', | ||
| }), | ||
| ).toBe('left:0;margin:16px;opacity:0.5;padding:4px'); | ||
| var styles = { |
There was a problem hiding this comment.
I left a few tests here that specifically assert things about markup (e.g. that it doesn't have extra px or spaces).
Those that only assert correctness are replaced with integration tests.
| @@ -90,6 +90,9 @@ var DOMPropertyOperations = { | |||
| if (value === '') { | |||
| return true; | |||
| } | |||
| if (value === '' + expected) { | |||
There was a problem hiding this comment.
If expected is null, and the attribute is the string "null" then this should not match.
There was a problem hiding this comment.
See the use of shouldIgnoreValue before the equivalent use case below.
It still touches internals but it tests both renderers.
Some of these are handy as integration tests so I moved them there. But some test markup specifically so I changed them to use DOMServer.
I forked createMarkupForStyles() into ReactDOMComponent and ReactPartialRenderer. Duplication is fine because one of them will soon be gone (guess which one!) The warnInvalidStyle helper is used by both server and client, unlike other client-only stuff in CSSPropertyOperations, so I moved it to a separately module used in both.
|
I hope 6929d61 fixed this? |
| var warning = require('fbjs/lib/warning'); | ||
|
|
||
| // This is currently duplicated with DOMMarkupOperations. | ||
| // TODO: Find a better place for this. |
There was a problem hiding this comment.
Nit: maybe include a comment denoting the end of the duplicated code; at first I thought it was just the regex that was duped.
| @@ -467,16 +503,44 @@ describe('ReactDOMServerIntegration', () => { | |||
| }); | |||
|
|
|||
| // this probably is just masking programmer error, but it is existing behavior. | |||
| itRenders('className prop with true value', async render => { | |||
| itRenders('htmlFor prop with true value', async render => { | |||
There was a problem hiding this comment.
Oops, this was almost certainly a copy-paste error on my part. Thanks for fixing it!
| @@ -111,6 +114,38 @@ function validateDangerousTag(tag) { | |||
| } | |||
| } | |||
|
|
|||
| var processStyleName = memoizeStringOnly(function(styleName) { | |||
There was a problem hiding this comment.
There should probably be a comment here noting that the code here is copied in ReactDOMComponent.
There was a problem hiding this comment.
We'll delete it in a few days so probably not worth it.
This forks
DOMPropertyOperationsinto two different files:DOMPropertyOperationsis DOM-only andDOMMarkupOperationsis SSR-only. This way we don’t have to bundle DOM-only code paths into the SSR renderer. I guess ES6 modules would also help here but I didn’t want to wait. I also rewrote markup-related unit tests as integration tests.In last few commits I also rewrote tests for CSS properties in public API, and split markup generation (server only), warning about key-value pairs (shared), and setting properties and validation (client-only).