Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Warn if the included mixin is undefined #6158

Merged
merged 1 commit into from
Jun 26, 2016
+103 −0
Merged

Warn if the included mixin is undefined #6158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 15 additions & 0 deletions src/isomorphic/classic/class/ReactClass.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,21 @@ function validateMethodOverride(isAlreadyDefined, name) {
*/
function mixSpecIntoComponent(Constructor, spec) {
if (!spec) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gaearon Since we have this condition, a mixin in case if is an array, will never get inside this block.
I feel if a mixin is an array, then its better we show a separate warning that says:

Warning: Your\'re attempting to include a mixin that is an array. Expected object but got array

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see. Never mind then, I retract my previous comment 👍

if (__DEV__) {
var typeofSpec = typeof spec;
var isMixinValid = typeofSpec === 'object' && spec !== null;

warning(
isMixinValid,
'%s: You\'re attempting to include a mixin that is either null ' +
'or not an object. Check the mixins included by the component, ' +
'as well as any mixins they include themselves. ' +
'Expected object but got %s.',
Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I can see, this will print Expected object but got object. for arrays.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah right. IMHO, I don't think there might be a case where an array will be included as mixin? The docs also has an example with a mixin being an object. Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean that we should avoid confusing warnings even if turn up in edge cases.
We could do something like

var type;
if (spec == null) {
  type = '' + spec;
} else if (Array.isArray(spec)) {
  type = 'array';
} else {
  type = typeof spec;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

As noted below, you’re right—we’ll never get into this block with arrays anyway so it doesn’t matter. I was wrong!

Constructor.displayName || 'ReactClass',
spec === null ? null : typeofSpec
);
}

return;
}

Expand Down
88 changes: 88 additions & 0 deletions src/isomorphic/classic/class/__tests__/ReactClassMixin-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,94 @@ describe('ReactClass-mixin', function() {
);
});

it('should warn if the mixin is undefined', function() {
spyOn(console, 'error');

React.createClass({
mixins: [undefined],

render: function() {
return <span />;
},
});

expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toBe(
'Warning: ReactClass: You\'re attempting to include a mixin that is ' +
'either null or not an object. Check the mixins included by the ' +
'component, as well as any mixins they include themselves. ' +
'Expected object but got undefined.'
);
});

it('should warn if the mixin is null', function() {
spyOn(console, 'error');

React.createClass({
mixins: [null],

render: function() {
return <span />;
},
});

expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toBe(
'Warning: ReactClass: You\'re attempting to include a mixin that is ' +
'either null or not an object. Check the mixins included by the ' +
'component, as well as any mixins they include themselves. ' +
'Expected object but got null.'
);
});

it('should warn if an undefined mixin is included in another mixin', function() {
spyOn(console, 'error');

var mixinA = {
mixins: [undefined],
};

React.createClass({
mixins: [mixinA],

render: function() {
return <span />;
},
});

expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toBe(
'Warning: ReactClass: You\'re attempting to include a mixin that is ' +
'either null or not an object. Check the mixins included by the ' +
'component, as well as any mixins they include themselves. ' +
'Expected object but got undefined.'
);
});

it('should warn if a null mixin is included in another mixin', function() {
spyOn(console, 'error');

var mixinA = {
mixins: [null],
};

React.createClass({
mixins: [mixinA],

render: function() {
return <span />;
},
});

expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toBe(
'Warning: ReactClass: You\'re attempting to include a mixin that is ' +
'either null or not an object. Check the mixins included by the ' +
'component, as well as any mixins they include themselves. ' +
'Expected object but got null.'
);
});

it('should throw if the mixin is a React component', function() {
expect(function() {
React.createClass({
Expand Down