Fix false "unknown property" warnings for events in SSR #10272
Conversation
gaearon
force-pushed
the
fix-ssr-injection
branch
from
afc71a4 to
07c9fc4
Compare
|
I split the fix in two commits now. The first one fixes an issue with our integration tests. Client and server renderers used to share some modules (e.g. event plugin list), and so the test was not representative of what happened in the wild. I added
|
Client and server renderers still share some modules, including injections. This means they have shared state in this test, potentially missing issues that would occur in real world. After this change, the client and server modules are completely isolated in the test. This makes the tests fail due to facebook#10265 (a real issue that was missed).
gaearon
force-pushed
the
fix-ssr-injection
branch
from
07c9fc4 to
0352c93
Compare
gaearon
force-pushed
the
fix-ssr-injection
branch
from
0352c93 to
c58ae24
Compare
|
After chatting with @spicyj I pushed a different fix. Instead of injecting the plugins in DEV, I skip |
There was a problem hiding this comment.
I think the change to ReactDOMUnknownPropertyHook looks reasonable.
The ReactDOMServerIntegration-test is pretty dense and hard to follow. Too many levels of chained method calls between what we're testing and asserting, in this case. I tried to poke around a bit and verify that I can break the test but I didn't have much luck. I wonder if we should refactor this test to be more verbose but easier to follow?
Just an observation, but this test takes ~30 seconds to run in master and ~50 seconds to run in this branch.
We can probably split it into multiple tests. Then multiple workers can run it instead of blocking a single worker for a super giant test file. |
(Follow up: we chatted and couldn’t reproduce this issue. The test seems to fail consistently if unknown attributes are added.) |
| // TODO: this is a hack for testing dynamic injection. Remove this when we decide | ||
| // how to do static injection instead. | ||
| // Now we reset the modules again to load the server renderer. | ||
| // Resetting is important because we want to avoid any shared state |
| if (EventPluginRegistry.registrationNameModules.hasOwnProperty(name)) { | ||
| return true; | ||
| } | ||
| if (EventPluginRegistry.plugins.length === 0 && name.indexOf('on') === 0) { |
There was a problem hiding this comment.
I think it would be nice to do /^on[A-Z]/.test(name) here -- that is, only counting properties with a capital letter after "on". Then onions={true} is caught. Since this is SSR only I guess it isn't a big deal.
lgtm otherwise, thanks!
There was a problem hiding this comment.
Oops, thanks for pointing it out! I totally did not think about the onions. 😛
There was a problem hiding this comment.
FWIW I considered this but thought it wasn't worth the tradeoff- (regex is much slower)- given that the DOM render would warn about onions when the client took over.
There was a problem hiding this comment.
Regex shouldn't be that much slower in practice.
There was a problem hiding this comment.
Yeah, it's probably not enough of a difference to actually matter. And it's dev-only anyway I guess.
4 participants
This fixes #10265.
Verified by running SSR fixture with the fix (I no longer see the erroneous warnings).
I think we should get this in before the beta because otherwise SSR is somewhat unusable due to the false positives on every event.
The fix itself is rather gross. But it‘s the minimal one that works, and I don’t want to change too much before the beta. We know it will work because in principle it just reverts the DEV build to what it was before #10173.I will start working on an alternative fix that forks injections instead of reusing them now. But I think this is an easier/safer thing to get in now.I’ll also need to understand why tests didn’t catch this. Probably because event plugins are in a shared module so the list is singleton for both ReactDOM and ReactDOMServer in tests. And they share it even though actual bundles would not.I understand it now. See followup comment.