Skip to content

Commit

Permalink
Start warning when React.DOM.x is passed into JSX
Browse files Browse the repository at this point in the history
React.DOM is becoming helper factories to generate ReactElements. They're not
classes. It will be ok to call them directly as functions, but not to use them
where a class is expected.
  • Loading branch information
sebmarkbage authored and zpao committed Oct 7, 2014
1 parent 15a0c89 commit 7f9b1d1
Show file tree
Hide file tree
Showing 8 changed files with 86 additions and 29 deletions.
4 changes: 2 additions & 2 deletions src/browser/ReactDOM.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,11 @@ var mapObject = require('mapObject');
*/
function createDOMFactory(tag) {
if (__DEV__) {
return ReactLegacyDescriptor.wrapFactory(
return ReactLegacyDescriptor.markNonLegacyFactory(
ReactDescriptorValidator.createFactory(tag)
);
}
return ReactLegacyDescriptor.wrapFactory(
return ReactLegacyDescriptor.markNonLegacyFactory(
ReactDescriptor.createFactory(tag)
);
}
Expand Down
31 changes: 29 additions & 2 deletions src/browser/__tests__/ReactDOM-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ var React = require('React');
var ReactDOM = require('ReactDOM');
var ReactMount = require('ReactMount');
var ReactTestUtils = require('ReactTestUtils');
var div = React.createFactory(ReactDOM.div); // TODO: use string
var div = React.createFactory('div');

describe('ReactDOM', function() {
// TODO: uncomment this test once we can run in phantom, which
Expand Down Expand Up @@ -117,6 +117,33 @@ describe('ReactDOM', function() {
});

it('should be a valid class', function() {
expect(React.isValidClass(ReactDOM.div)).toBe(true);
expect(React.isValidClass(ReactDOM.div)).toBe(false);
});

it('allow React.DOM factories to be called without warnings', function() {
spyOn(console, 'warn');
var descriptor = React.DOM.div();
expect(descriptor.type).toBe('div');
expect(console.warn.argsForCall.length).toBe(0);
});

it('warns but allow dom factories to be used in createFactory', function() {
spyOn(console, 'warn');
var factory = React.createFactory(React.DOM.div);
expect(factory().type).toBe('div');
expect(console.warn.argsForCall.length).toBe(1);
expect(console.warn.argsForCall[0][0]).toContain(
'Do not pass React.DOM.div'
);
});

it('warns but allow dom factories to be used in createElement', function() {
spyOn(console, 'warn');
var descriptor = React.createElement(React.DOM.div);
expect(descriptor.type).toBe('div');
expect(console.warn.argsForCall.length).toBe(1);
expect(console.warn.argsForCall[0][0]).toContain(
'Do not pass React.DOM.div'
);
});
});
2 changes: 1 addition & 1 deletion src/browser/ui/React.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ var React = {
renderComponentToStaticMarkup:
ReactServerRendering.renderComponentToStaticMarkup,
unmountComponentAtNode: ReactMount.unmountComponentAtNode,
isValidClass: ReactDescriptor.isValidFactory,
isValidClass: ReactLegacyDescriptor.isValidFactory,
isValidComponent: ReactDescriptor.isValidDescriptor,
withContext: ReactContext.withContext,
__internals: {
Expand Down
2 changes: 1 addition & 1 deletion src/browser/ui/ReactMount.js
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ var ReactMount = {
typeof nextDescriptor === 'string' ?
' Instead of passing an element string, make sure to instantiate ' +
'it by passing it to React.createElement.' :
ReactDescriptor.isValidFactory(nextDescriptor) ?
ReactLegacyDescriptor.isValidFactory(nextDescriptor) ?
' Instead of passing a component class, make sure to instantiate ' +
'it by passing it to React.createElement.' :
// Check if it quacks like a descriptor
Expand Down
2 changes: 1 addition & 1 deletion src/core/ReactCompositeComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,7 @@ function mixSpecIntoComponent(Constructor, spec) {
}

invariant(
!ReactDescriptor.isValidFactory(spec),
!ReactLegacyDescriptor.isValidFactory(spec),
'ReactCompositeComponent: You\'re attempting to ' +
'use a component class as a mixin. Instead, just use a regular object.'
);
Expand Down
17 changes: 0 additions & 17 deletions src/core/ReactDescriptor.js
Original file line number Diff line number Diff line change
Expand Up @@ -214,23 +214,6 @@ ReactDescriptor.cloneAndReplaceProps = function(oldDescriptor, newProps) {
return newDescriptor;
};

/**
* Checks if a value is a valid descriptor constructor.
*
* @param {*}
* @return {boolean}
* @public
*/
ReactDescriptor.isValidFactory = function(factory) {
return typeof factory === 'function' && (
typeof factory.type === 'string' || (
typeof factory.type === 'function' &&
typeof factory.type.prototype.mountComponent === 'function' &&
typeof factory.type.prototype.receiveComponent === 'function'
)
);
};

/**
* @param {?object} object
* @return {boolean} True if `object` is a valid component.
Expand Down
54 changes: 51 additions & 3 deletions src/core/ReactLegacyDescriptor.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
"use strict";

var ReactCurrentOwner = require('ReactCurrentOwner');
var ReactDescriptor = require('ReactDescriptor');

var invariant = require('invariant');
var monitorCodeUse = require('monitorCodeUse');
Expand Down Expand Up @@ -78,6 +77,14 @@ function warnForPlainFunctionType(type) {
}
}

function warnForNonLegacyFactory(type) {
warning(
false,
'Do not pass React.DOM.' + type.type + ' to JSX or createFactory. ' +
'Use the string "' + type + '" instead.'
);
}

/**
* Transfer static properties from the source to the target. Functions are
* rebound to have this reflect the original source.
Expand Down Expand Up @@ -109,6 +116,7 @@ function proxyStaticMethods(target, source) {
// We use an object instead of a boolean because booleans are ignored by our
// mocking libraries when these factories gets mocked.
var LEGACY_MARKER = {};
var NON_LEGACY_MARKER = {};

var ReactLegacyDescriptorFactory = {};

Expand All @@ -119,6 +127,16 @@ ReactLegacyDescriptorFactory.wrapCreateFactory = function(createFactory) {
return createFactory(type);
}

if (type.isReactNonLegacyFactory) {
// This is probably a factory created by ReactDOM we unwrap it to get to
// the underlying string type. It shouldn't have been passed here so we
// warn.
if (__DEV__) {
warnForNonLegacyFactory(type);
}
return createFactory(type.type);
}

if (type.isReactLegacyFactory) {
// This is probably a legacy factory created by ReactCompositeComponent.
// We unwrap it to get to the underlying class.
Expand All @@ -143,6 +161,20 @@ ReactLegacyDescriptorFactory.wrapCreateDescriptor = function(createDescriptor) {
return createDescriptor.apply(this, arguments);
}

var args;

if (type.isReactNonLegacyFactory) {
// This is probably a factory created by ReactDOM we unwrap it to get to
// the underlying string type. It shouldn't have been passed here so we
// warn.
if (__DEV__) {
warnForNonLegacyFactory(type);
}
args = Array.prototype.slice.call(arguments, 0);
args[0] = type.type;
return createDescriptor.apply(this, args);
}

if (type.isReactLegacyFactory) {
// This is probably a legacy factory created by ReactCompositeComponent.
// We unwrap it to get to the underlying class.
Expand All @@ -152,7 +184,7 @@ ReactLegacyDescriptorFactory.wrapCreateDescriptor = function(createDescriptor) {
// future proofs unit testing that assume that these are classes.
type.type._mockedReactClassConstructor = type;
}
var args = Array.prototype.slice.call(arguments, 0);
args = Array.prototype.slice.call(arguments, 0);
args[0] = type.type;
return createDescriptor.apply(this, args);
}
Expand All @@ -170,7 +202,7 @@ ReactLegacyDescriptorFactory.wrapCreateDescriptor = function(createDescriptor) {

ReactLegacyDescriptorFactory.wrapFactory = function(factory) {
invariant(
ReactDescriptor.isValidFactory(factory),
typeof factory === 'function',
'This is suppose to accept a descriptor factory'
);
var legacyDescriptorFactory = function(config, children) {
Expand All @@ -186,6 +218,22 @@ ReactLegacyDescriptorFactory.wrapFactory = function(factory) {
return legacyDescriptorFactory;
};

// This is used to mark a factory that will remain. E.g. we're allowed to call
// it as a function. However, you're not suppose to pass it to createElement
// or createFactory, so it will warn you if you do.
ReactLegacyDescriptorFactory.markNonLegacyFactory = function(factory) {
factory.isReactNonLegacyFactory = NON_LEGACY_MARKER;
return factory;
};

// Checks if a factory function is actually a legacy factory pretending to
// be a class.
ReactLegacyDescriptorFactory.isValidFactory = function(factory) {
// TODO: This will be removed and moved into a class validator or something.
return typeof factory === 'function' &&
factory.isReactLegacyFactory === LEGACY_MARKER;
};

ReactLegacyDescriptorFactory._isLegacyCallWarningEnabled = true;

module.exports = ReactLegacyDescriptorFactory;
3 changes: 1 addition & 2 deletions src/test/ReactTestUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ var EventPluginHub = require('EventPluginHub');
var EventPropagators = require('EventPropagators');
var React = require('React');
var ReactDescriptor = require('ReactDescriptor');
var ReactDOM = require('ReactDOM');
var ReactBrowserEventEmitter = require('ReactBrowserEventEmitter');
var ReactMount = require('ReactMount');
var ReactTextComponent = require('ReactTextComponent');
Expand Down Expand Up @@ -242,7 +241,7 @@ var ReactTestUtils = {
var ConvenienceConstructor = React.createClass({
render: function() {
return React.createElement(
ReactDOM[mockTagName], // TODO: Replace this with just a string
mockTagName,
null,
this.props.children
);
Expand Down

0 comments on commit 7f9b1d1

Please sign in to comment.