[Not for Merge] Investigate CI failures on Symbol/Function warning PR #13453
Conversation
|
ReactDOM: size: 🔺+0.2%, gzip: 🔺+0.1% Details of bundled changes.Comparing: 146c9fb...7fac775 react-dom
Generated by 🚫 dangerJS |
nhunzaker
changed the title
|
I might be worth merging some SSR integration tests together. Each individual test case generates 4 Jest tests. I think they're unnecessarily isolated right now. Maybe we can have less tests where each test checks more than a single property. |
|
That seems like it's the way to go. I still need to trace memory allocation, but most of the execution time cost of these tests seems to be the result of calling Consolidating similar tests is probably the easiest fix, but I'm going to also try to gather some more info about memory to see if there's a more systemic fix. |
|
We’ve talked more with @sebmarkbage about this and he raised a good point. It seems like overall treating them consistently is adding significant overhead in the implementation readability. And there’s undoubtedly runtime overhead to it too. However we already warn about those cases. So what if we pivot into the opposite direction? For bad inputs, ensure we warn — and do the least amount of work possible. It doesn’t have to be consistent if we print a warning, and we reserve the right to change warned-about behavior between patch versions. In that world, what would an ideal implementation look like? |
|
Ahh neat, @rickhanlonii added a memory leak feature to Jest and, indeed, something is leaking. This is the best! |
|
I might be thinking too narrowly about this, but I think a good step towards "least amount of work" could be to move property warnings into single call upfront, like where Working on this code, dealing with validators later in the renderer was the biggest source of frustration. For example: when does a textarea validate on Having a clear validation step up could mitigate a lot of that. |
|
Beyond that, for the purposes of the SSR tests, it seems like |
|
Looks like Jest thinks that there is a memory leak in the |
| | `defaultValue=(null)`| (initial, ssr warning)| `<empty string>` | | ||
| | `defaultValue=(symbol)`| (initial, warning)| `<empty string>` | | ||
| | `defaultValue=(function)`| (initial, warning)| `<empty string>` | | ||
| | `defaultValue=(null)`| (initial)| `<empty string>` | | ||
| | `defaultValue=(undefined)`| (initial)| `<empty string>` | | ||
|
|
||
| ## `defaultValuE` (on `<input>` inside `<div>`) |
There was a problem hiding this comment.
I think this is intentional, to catch bad casing of defaultValue.
|
I think I've found the memory leak, there's some shared code in the closures for https://github.com/facebook/react/blob/master/packages/react-dom/src/__tests__/utils/ReactDOMServerIntegrationTestUtils.js#L263-L285. I'm going to see if I can stamp it out. Also I'll close this out. I think I have a good direction, and I don't need to blast CI :) |
#13394 fails CI due to a memory issue. I'm using this PR to slowly peal back the changes and see where the issue might be.
Here goes...