Properly warn on Symbol/Function in SSR, eliminate defaultValue and defaultChecked reserved words, fix some SSR mismatches #13394
Conversation
| | `value=(symbol)`| (changed, error, warning, ssr error)| `` | | ||
| | `value=(function)`| (changed, warning, ssr warning)| `"function f() {}"` | | ||
| | `value=(symbol)`| (initial, warning, ssr error, ssr mismatch)| `<empty string>` | | ||
| | `value=(function)`| (initial, warning, ssr mismatch)| `<empty string>` | |
There was a problem hiding this comment.
Why do we have ssr error and ssr mismatch here but not in the other one? Can we fix SSR too?
There was a problem hiding this comment.
Yeah, I'm curious about that too. Writing a test for it now.
| @@ -1237,10 +1244,6 @@ class ReactDOMServerRenderer { | |||
| } | |||
| } | |||
|
|
|||
| if (__DEV__) { | |||
| validatePropertiesInDevelopment(tag, props); | |||
| } | |||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| @@ -831,7 +833,7 @@ class ReactDOMServerRenderer { | |||
| parentNamespace: string, | |||
| ): string { | |||
| if (typeof child === 'string' || typeof child === 'number') { | |||
| const text = '' + child; | |||
| const text = toString(getToStringValue(child)); | |||
There was a problem hiding this comment.
I don't like how both of these functions are essential to safely cast values to strings. I'd like to also export a function that just calls both of these.
There was a problem hiding this comment.
We should probably get rid of this interim value completely. I’ve added it because we used this pattern in the input code but I feel like it caused a lot of confusion already 😕
Maybe we can keep the original value as mixed in the wrapper state?
I can work on that next week.
|
ReactDOM: size: 🔺+0.1%, gzip: 🔺+0.2% Details of bundled changes.Comparing: f6fb03e...da338d3 react-dom
react-art
react-test-renderer
react-noop-renderer
react-reconciler
react-native-renderer
schedule
Generated by 🚫 dangerJS |
|
@gaearon making headway, but it looks like SSR needs a few more changes to make the warning consistent. I need to shift gears, but I'll circle back. |
|
I'm wary of how much mental abstraction overhead it feels like |
| @@ -11855,21 +11855,21 @@ | |||
| | `value=(empty string)`| (initial)| `<empty string>` | | |||
| | `value=(array with string)`| (changed)| `"string"` | | |||
| | `value=(empty array)`| (initial)| `<empty string>` | | |||
| | `value=(object)`| (changed)| `"result of toString()"` | | |||
| | `value=(object)`| (changed, ssr error, ssr mismatch)| `"result of toString()"` | | |||
There was a problem hiding this comment.
This should line up, adding unit test.
|
Working through this, but the trend is that we don't need to rely so much on the |
| } | ||
|
|
||
| props = Object.assign({}, props, { | ||
| value: undefined, | ||
| children: '' + initialValue, | ||
| children: initialValue, |
There was a problem hiding this comment.
This appears to produce the effect I want, but it's gross. Now that it works as I think it should, I'll work on making the code better.
| | `value=(symbol)`| (changed, error, warning, ssr error)| `` | | ||
| | `value=(function)`| (changed, warning, ssr warning)| `"function f() {}"` | | ||
| | `value=(symbol)`| (initial, warning)| `<empty string>` | | ||
| | `value=(function)`| (initial, warning)| `<empty string>` | |
There was a problem hiding this comment.
I'm not sure if I'm interpreting this correctly. I'm working off of:
In any case, this appears to fix a behavior where functions were stringified into textareas instead of converting into empty strings. There is no longer a mismatch.
Still I can't figure out why ssr warning went away, even though I can confirm the warning in unit tests.
| | `value=(symbol)`| (changed, error, warning, ssr mismatch)| `` | | ||
| | `value=(function)`| (changed, warning, ssr mismatch)| `"function f() {}"` | | ||
| | `value=(symbol)`| (initial, warning)| `<empty string>` | | ||
| | `value=(function)`| (initial, warning)| `<empty string>` | | ||
| | `value=(null)`| (initial)| `<empty string>` | | ||
| | `value=(undefined)`| (initial)| `<empty string>` | |
There was a problem hiding this comment.
Took care of quite a few mismatches :)
|
Okay! Validating the properties before processing them took care of all of the warnings, and not stringifying values appears to fix a lot of SSR mismatch issues too. I hope I've interpreted this correctly. I'd like to add some documentation to the attribute table to help clear that up (follow up PR) |
| | `value=(float)`| (changed)| `"99.99"` | | ||
| | `value=(true)`| (changed)| `"true"` | | ||
| | `value=(false)`| (changed)| `"false"` | | ||
| | `value=(string 'true')`| (changed)| `"true"` | | ||
| | `value=(string 'false')`| (changed)| `"false"` | | ||
| | `value=(string 'on')`| (changed)| `"on"` | | ||
| | `value=(string 'off')`| (changed)| `"off"` | | ||
| | `value=(symbol)`| (changed, error, warning, ssr error)| `` | | ||
| | `value=(function)`| (changed, warning, ssr warning)| `"function f() {}"` | | ||
| | `value=(null)`| (initial, warning, ssr warning)| `<empty string>` | |
There was a problem hiding this comment.
Trying to figure this one out. Unit tests suggest that a warning is firing using SSR. Does this mean that the attribute table didn't see a SSR warning?
There was a problem hiding this comment.
It's good when you don't have ssr warning. Just warning means everything is consistent between client and server.
ssr warning confusingly means "client and server match in behavior except warnings". So it's bad to have it.
nhunzaker
force-pushed
the
update-attribute-table-with-symbols
branch
from
0f6766e
to
c2bcd60
Compare
|
Hmm... Why is CI running out of memory... |
| @@ -222,6 +222,41 @@ describe('ReactDOMServerIntegration', () => { | |||
| expect(e.value).toBe('foo'); | |||
| }, | |||
| ); | |||
|
|
|||
| itRenders('a textarea with Symbol value with a warning', async render => { | |||
There was a problem hiding this comment.
What about passing a function as a value? Should we test that here as well?
Also, defaultValue?
There was a problem hiding this comment.
Definitely. Thanks!
There was a problem hiding this comment.
Great catch. Working through some changes now that actually eliminate all of our todos about warning on defaultValue.
nhunzaker
force-pushed
the
update-attribute-table-with-symbols
branch
from
c2bcd60
to
e168b66
Compare
|
Okay, this ended up being a bigger change than I realized, but I'm to the point where Also it looks like a lot of boolean attributes weren't sending warnings before. |
| ].forEach(function(name) { | ||
| properties[name] = new PropertyInfoRecord( | ||
| name, | ||
| BOOLEANISH_STRING, |
There was a problem hiding this comment.
Yikes, this should be OVERLOADED_BOOLEAN
There was a problem hiding this comment.
I think this is to the point where it needs to be reviewed. Build failure is because CI runs out of memory:
Security context: 0x877bda25879 <JSObject>
1: requireModule [/home/circleci/project/node_modules/jest-runtime/build/index.js:~309] [pc=0x2390cf094548](this=0x45e22389941 <Runtime map = 0x1b6113890011>,from=0x1037f9641259 <String[56]: /home/circleci/project/packages/events/EventPluginHub.js>,moduleName=0xea6753597c9 <String[25]: shared/reactProdInvariant>,options=0x29972df822d1 <undefined>)
2: arguments adaptor frame: 2->3
3: r...
FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory
The server integration tests are really heavy. I wonder if all of the extra tests in this suite are to blame.
| trapBubbledEvent(TOP_INVALID, domElement); | ||
| // For controlled components we always need to ensure we're listening | ||
| // to onChange. Even if there is no listener. | ||
| ensureListeningTo(rootContainerElement, 'onChange'); | ||
| break; | ||
| case 'textarea': | ||
| ReactDOMFiberTextarea.initWrapperState(domElement, rawProps); | ||
| props = ReactDOMFiberTextarea.getHostProps(domElement, rawProps); |
There was a problem hiding this comment.
With this addition, I think this set of operations is exactly the same for hydration and client-rendering. Also I think they need to be the same for SSR to align warning behavior. Maybe these should be in a shared function.
|
Looks like the boolean warnings weren't a result of this PR (thankfully; this was bugging me). I sent out a PR for that here: We don't have to merge it, but I'd appreciate it so that it's easier to see the effects on the attribute table of this PR. |
nhunzaker
force-pushed
the
update-attribute-table-with-symbols
branch
from
6a8a9af
to
9c62214
Compare
|
Time to rename the PR? :-) |
nhunzaker
changed the title
|
Wasn't quite sure how to summarize it, so I just wrote everything |
nhunzaker
changed the title
|
Is it ready for review? |
|
Assuming this approach is acceptable, I think this is good to merge. |
In facebook#13394, I encountered an issue where the ReactDOMServerIntegrationForm test suite consumed sufficient memory to crash CircleCI. Breaking up this test suite by form element type resolved the issue. This commit performs that change separate from the Symbol/Function stringification changes in facebook#13394.
In facebook#13394, I encountered an issue where the ReactDOMServerIntegrationForm test suite consumed sufficient memory to crash CircleCI. Breaking up this test suite by form element type resolved the issue. This commit performs that change separate from the Symbol/Function stringification changes in facebook#13394.
In #13394, I encountered an issue where the ReactDOMServerIntegrationForm test suite consumed sufficient memory to crash CircleCI. Breaking up this test suite by form element type resolved the issue. This commit performs that change separate from the Symbol/Function stringification changes in #13394.
nhunzaker
force-pushed
the
update-attribute-table-with-symbols
branch
5 times, most recently
from
7342c79
to
9a33f99
Compare
|
|
||
| let options; | ||
| beforeEach(function() { |
There was a problem hiding this comment.
Didn't catch this in the rebase, patching up.
|
Everything is up to date with master (including the ReactDOMServerIntegrationForm-test split). I've also verified that there is no change to the attribute table from updates on master. |
* Add to-string protection to SSR * Add validation step to inputs * Only validate textarea value prop * Properly handle true/false and NaN * Simplify validation by doing it ahead of time * Tweak toStringValue comment * defaultValue and defaultChecked are no longer reserved words * Use correct type for defaultValue and defaultChecked * Add test coverage and fixes for mixed type selection on options * Improve comment about backwards compatibility with defaultValue * Align defaultValue and defaultChecked with value and checked
nhunzaker
force-pushed
the
update-attribute-table-with-symbols
branch
from
30dc406
to
da338d3
Compare
In facebook#13394, I encountered an issue where the ReactDOMServerIntegrationForm test suite consumed sufficient memory to crash CircleCI. Breaking up this test suite by form element type resolved the issue. This commit performs that change separate from the Symbol/Function stringification changes in facebook#13394.
In facebook#13394, I encountered an issue where the ReactDOMServerIntegrationForm test suite consumed sufficient memory to crash CircleCI. Breaking up this test suite by form element type resolved the issue. This commit performs that change separate from the Symbol/Function stringification changes in facebook#13394.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution. |
|
Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you! |
Updates attribute table based on the latest Symbol stringification work.