@samitouri / QOS-React-2 / commits / 7f9b1d18d1

Start warning when React.DOM.x is passed into JSX

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.

Sebastian Markbage committed Oct 7, 2014 at 10:26 UTC 7f9b1d18d197d102777f214e782ecc27685cf481

8 files changed +86 -29

src/browser/ReactDOM.js

+2 -2

@@ -33,11 +33,11 @@ var mapObject = require('mapObject');

33 */

34 function createDOMFactory(tag) {

35 if (__DEV__) {

36 - return ReactLegacyDescriptor.wrapFactory(

36 + return ReactLegacyDescriptor.markNonLegacyFactory(

37 ReactDescriptorValidator.createFactory(tag)

38 );

39 }

40 - return ReactLegacyDescriptor.wrapFactory(

40 + return ReactLegacyDescriptor.markNonLegacyFactory(

41 ReactDescriptor.createFactory(tag)

42 );

43 }

src/browser/__tests__/ReactDOM-test.js

+29 -2

@@ -25,7 +25,7 @@ var React = require('React');

25 var ReactDOM = require('ReactDOM');

26 var ReactMount = require('ReactMount');

27 var ReactTestUtils = require('ReactTestUtils');

28 -var div = React.createFactory(ReactDOM.div); // TODO: use string

28 +var div = React.createFactory('div');

30 describe('ReactDOM', function() {

31 // TODO: uncomment this test once we can run in phantom, which

@@ -117,6 +117,33 @@ describe('ReactDOM', function() {

117 });

118

119 it('should be a valid class', function() {

120 - expect(React.isValidClass(ReactDOM.div)).toBe(true);

120 + expect(React.isValidClass(ReactDOM.div)).toBe(false);

121 + });

122 +

123 + it('allow React.DOM factories to be called without warnings', function() {

124 + spyOn(console, 'warn');

125 + var descriptor = React.DOM.div();

126 + expect(descriptor.type).toBe('div');

127 + expect(console.warn.argsForCall.length).toBe(0);

128 + });

129 +

130 + it('warns but allow dom factories to be used in createFactory', function() {

131 + spyOn(console, 'warn');

132 + var factory = React.createFactory(React.DOM.div);

133 + expect(factory().type).toBe('div');

134 + expect(console.warn.argsForCall.length).toBe(1);

135 + expect(console.warn.argsForCall[0][0]).toContain(

136 + 'Do not pass React.DOM.div'

137 + );

138 + });

139 +

140 + it('warns but allow dom factories to be used in createElement', function() {

141 + spyOn(console, 'warn');

142 + var descriptor = React.createElement(React.DOM.div);

143 + expect(descriptor.type).toBe('div');

144 + expect(console.warn.argsForCall.length).toBe(1);

145 + expect(console.warn.argsForCall[0][0]).toContain(

146 + 'Do not pass React.DOM.div'

147 + );

148 });

149 });

src/browser/ui/React.js

+1 -1

@@ -90,7 +90,7 @@ var React = {

90 renderComponentToStaticMarkup:

91 ReactServerRendering.renderComponentToStaticMarkup,

92 unmountComponentAtNode: ReactMount.unmountComponentAtNode,

93 - isValidClass: ReactDescriptor.isValidFactory,

93 + isValidClass: ReactLegacyDescriptor.isValidFactory,

94 isValidComponent: ReactDescriptor.isValidDescriptor,

95 withContext: ReactContext.withContext,

96 __internals: {

src/browser/ui/ReactMount.js

+1 -1

@@ -348,7 +348,7 @@ var ReactMount = {

348 typeof nextDescriptor === 'string' ?

349 ' Instead of passing an element string, make sure to instantiate ' +

350 'it by passing it to React.createElement.' :

351 - ReactDescriptor.isValidFactory(nextDescriptor) ?

351 + ReactLegacyDescriptor.isValidFactory(nextDescriptor) ?

352 ' Instead of passing a component class, make sure to instantiate ' +

353 'it by passing it to React.createElement.' :

354 // Check if it quacks like a descriptor

src/core/ReactCompositeComponent.js

+1 -1

@@ -471,7 +471,7 @@ function mixSpecIntoComponent(Constructor, spec) {

471 }

472

473 invariant(

474 - !ReactDescriptor.isValidFactory(spec),

474 + !ReactLegacyDescriptor.isValidFactory(spec),

475 'ReactCompositeComponent: You\'re attempting to ' +

476 'use a component class as a mixin. Instead, just use a regular object.'

477 );

src/core/ReactDescriptor.js

-17

@@ -214,23 +214,6 @@ ReactDescriptor.cloneAndReplaceProps = function(oldDescriptor, newProps) {

214 return newDescriptor;

215 };

216

217 -/**

218 - * Checks if a value is a valid descriptor constructor.

219 - *

220 - * @param {*}

221 - * @return {boolean}

222 - * @public

223 - */

224 -ReactDescriptor.isValidFactory = function(factory) {

225 - return typeof factory === 'function' && (

226 - typeof factory.type === 'string' || (

227 - typeof factory.type === 'function' &&

228 - typeof factory.type.prototype.mountComponent === 'function' &&

229 - typeof factory.type.prototype.receiveComponent === 'function'

230 - )

231 - );

232 -};

233 -

217 /**

218 * @param {?object} object

219 * @return {boolean} True if `object` is a valid component.

src/core/ReactLegacyDescriptor.js

+51 -3

@@ -19,7 +19,6 @@

19 "use strict";

21 var ReactCurrentOwner = require('ReactCurrentOwner');

22 -var ReactDescriptor = require('ReactDescriptor');

23 var invariant = require('invariant');

24 var monitorCodeUse = require('monitorCodeUse');

@@ -78,6 +77,14 @@ function warnForPlainFunctionType(type) {

77 }

78 }

80 +function warnForNonLegacyFactory(type) {

81 + warning(

82 + false,

83 + 'Do not pass React.DOM.' + type.type + ' to JSX or createFactory. ' +

84 + 'Use the string "' + type + '" instead.'

85 + );

86 +}

87 +

88 /**

89 * Transfer static properties from the source to the target. Functions are

90 * rebound to have this reflect the original source.

@@ -109,6 +116,7 @@ function proxyStaticMethods(target, source) {

116 // We use an object instead of a boolean because booleans are ignored by our

117 // mocking libraries when these factories gets mocked.

118 var LEGACY_MARKER = {};

119 +var NON_LEGACY_MARKER = {};

120

121 var ReactLegacyDescriptorFactory = {};

122

@@ -119,6 +127,16 @@ ReactLegacyDescriptorFactory.wrapCreateFactory = function(createFactory) {

127 return createFactory(type);

128 }

129

130 + if (type.isReactNonLegacyFactory) {

131 + // This is probably a factory created by ReactDOM we unwrap it to get to

132 + // the underlying string type. It shouldn't have been passed here so we

133 + // warn.

134 + if (__DEV__) {

135 + warnForNonLegacyFactory(type);

136 + }

137 + return createFactory(type.type);

138 + }

139 +

140 if (type.isReactLegacyFactory) {

141 // This is probably a legacy factory created by ReactCompositeComponent.

142 // We unwrap it to get to the underlying class.

@@ -143,6 +161,20 @@ ReactLegacyDescriptorFactory.wrapCreateDescriptor = function(createDescriptor) {

161 return createDescriptor.apply(this, arguments);

162 }

163

164 + var args;

165 +

166 + if (type.isReactNonLegacyFactory) {

167 + // This is probably a factory created by ReactDOM we unwrap it to get to

168 + // the underlying string type. It shouldn't have been passed here so we

169 + // warn.

170 + if (__DEV__) {

171 + warnForNonLegacyFactory(type);

172 + }

173 + args = Array.prototype.slice.call(arguments, 0);

174 + args[0] = type.type;

175 + return createDescriptor.apply(this, args);

176 + }

177 +

178 if (type.isReactLegacyFactory) {

179 // This is probably a legacy factory created by ReactCompositeComponent.

180 // We unwrap it to get to the underlying class.

@@ -152,7 +184,7 @@ ReactLegacyDescriptorFactory.wrapCreateDescriptor = function(createDescriptor) {

184 // future proofs unit testing that assume that these are classes.

185 type.type._mockedReactClassConstructor = type;

186 }

155 - var args = Array.prototype.slice.call(arguments, 0);

187 + args = Array.prototype.slice.call(arguments, 0);

188 args[0] = type.type;

189 return createDescriptor.apply(this, args);

190 }

@@ -170,7 +202,7 @@ ReactLegacyDescriptorFactory.wrapCreateDescriptor = function(createDescriptor) {

202

203 ReactLegacyDescriptorFactory.wrapFactory = function(factory) {

204 invariant(

173 - ReactDescriptor.isValidFactory(factory),

205 + typeof factory === 'function',

206 'This is suppose to accept a descriptor factory'

207 );

208 var legacyDescriptorFactory = function(config, children) {

@@ -186,6 +218,22 @@ ReactLegacyDescriptorFactory.wrapFactory = function(factory) {

218 return legacyDescriptorFactory;

219 };

220

221 +// This is used to mark a factory that will remain. E.g. we're allowed to call

222 +// it as a function. However, you're not suppose to pass it to createElement

223 +// or createFactory, so it will warn you if you do.

224 +ReactLegacyDescriptorFactory.markNonLegacyFactory = function(factory) {

225 + factory.isReactNonLegacyFactory = NON_LEGACY_MARKER;

226 + return factory;

227 +};

228 +

229 +// Checks if a factory function is actually a legacy factory pretending to

230 +// be a class.

231 +ReactLegacyDescriptorFactory.isValidFactory = function(factory) {

232 + // TODO: This will be removed and moved into a class validator or something.

233 + return typeof factory === 'function' &&

234 + factory.isReactLegacyFactory === LEGACY_MARKER;

235 +};

236 +

237 ReactLegacyDescriptorFactory._isLegacyCallWarningEnabled = true;

238

239 module.exports = ReactLegacyDescriptorFactory;

src/test/ReactTestUtils.js

+1 -2

@@ -23,7 +23,6 @@ var EventPluginHub = require('EventPluginHub');

23 var EventPropagators = require('EventPropagators');

24 var React = require('React');

25 var ReactDescriptor = require('ReactDescriptor');

26 -var ReactDOM = require('ReactDOM');

26 var ReactBrowserEventEmitter = require('ReactBrowserEventEmitter');

27 var ReactMount = require('ReactMount');

28 var ReactTextComponent = require('ReactTextComponent');

@@ -242,7 +241,7 @@ var ReactTestUtils = {

241 var ConvenienceConstructor = React.createClass({

242 render: function() {

243 return React.createElement(

245 - ReactDOM[mockTagName], // TODO: Replace this with just a string

244 + mockTagName,

245 null,

246 this.props.children

247 );