Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -281,28 +281,47 @@ function expectMarkupMismatch(serverElement, clientElement) {
return testMarkupMatch(serverElement, clientElement, false);
}

// TODO: this is a hack for testing dynamic injection. Remove this when we decide
// how to do static injection instead.
let onAfterResetModules = null;

// When there is a test that renders on server and then on client and expects a logged
// error, you want to see the error show up both on server and client. Unfortunately,
// React refuses to issue the same error twice to avoid clogging up the console.
// To get around this, we must reload React modules in between server and client render.
let onAfterResetModules = null;
function resetModules() {
// First, reset the modules to load the client renderer.
jest.resetModuleRegistry();
if (typeof onAfterResetModules === 'function') {
onAfterResetModules();
}

// TODO: can we express this test with only public API?
ExecutionEnvironment = require('ExecutionEnvironment');

PropTypes = require('prop-types');
React = require('react');
ReactDOM = require('react-dom');
ReactDOMServer = require('react-dom/server');
ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');
ReactDOMNodeStream = require('react-dom/node-stream');
ReactTestUtils = require('react-dom/test-utils');
// TODO: can we express this test with only public API?
ExecutionEnvironment = require('ExecutionEnvironment');

// 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(y)

// influencing the tests.
jest.resetModuleRegistry();
if (typeof onAfterResetModules === 'function') {
onAfterResetModules();
}

ReactDOMServer = require('react-dom/server');

// Finally, reset modules one more time before importing the stream renderer.
jest.resetModuleRegistry();
if (typeof onAfterResetModules === 'function') {
onAfterResetModules();
}

ReactDOMNodeStream = require('react-dom/node-stream');
}

describe('ReactDOMServerIntegration', () => {
Expand Down
5 changes: 5 additions & 0 deletions src/renderers/dom/shared/hooks/ReactDOMUnknownPropertyHook.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ if (__DEV__) {
if (EventPluginRegistry.registrationNameModules.hasOwnProperty(name)) {
return true;
}
if (EventPluginRegistry.plugins.length === 0 && name.indexOf('on') === 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, thanks for pointing it out! I totally did not think about the onions. 😛

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regex shouldn't be that much slower in practice.

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's probably not enough of a difference to actually matter. And it's dev-only anyway I guess.

https://jsperf.com/asdiohsd

// If no event plugins have been injected, we might be in a server environment.
// Don't check events in this case.
return true;
}
warnedProperties[name] = true;
var lowerCasedName = name.toLowerCase();

Expand Down