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.

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

Warn if the included mixin is undefined #6158

Merged
merged 1 commit into from
Jun 26, 2016

Conversation

swaroopsm
Copy link
Contributor

No description provided.

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@@ -437,6 +437,12 @@ function validateMethodOverride(isAlreadyDefined, name) {
*/
function mixSpecIntoComponent(Constructor, spec) {
if (!spec) {
warning(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should wrap this in a __DEV__ block.

@facebook-github-bot
Copy link

@swaroopsm updated the pull request.

var typeofSpec = typeof spec;

warning(
typeofSpec !== 'object' || typeofSpec === null,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is || typeofSpec === null protecting against?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh my bad. I meant spec === null.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I think the typeofSpec !== 'object' check here might be backwards. Warning behaves the opposite of an if statement, if I recall correctly. We should probably add a couple of unit tests to verify behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right. Warning behaves the opposite of an if statement.

@facebook-github-bot
Copy link

@swaroopsm updated the pull request.

@@ -437,6 +437,20 @@ function validateMethodOverride(isAlreadyDefined, name) {
*/
function mixSpecIntoComponent(Constructor, spec) {
if (!spec) {
if(__DEV__) {
var typeofSpec = typeof spec,
isMixinValid = typeofSpec === 'object' && spec !== null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just add another var statement, put a semicolon on the previous line.

@jimfb
Copy link
Contributor

jimfb commented Mar 1, 2016

One nit pick. Let's also add a few unit tests, and then I think this is ready to go.

@facebook-github-bot
Copy link

@swaroopsm updated the pull request.

@jimfb
Copy link
Contributor

jimfb commented Mar 1, 2016

Ok, looks good to me. Let's squash these commits (git rebase -i, git push -f) and I'll leave it open for a day to see if there is any other feedback before we merge.

@@ -437,6 +437,20 @@ function validateMethodOverride(isAlreadyDefined, name) {
*/
function mixSpecIntoComponent(Constructor, spec) {
if (!spec) {
if(__DEV__) {
Copy link
Member

Choose a reason for hiding this comment

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

Note: this should be failing lint (travis is really behind though 😢)

@zpao
Copy link
Member

zpao commented Mar 2, 2016

@jimfb before you merge, can you read through #1305 which we didn't merge in part due to issues with jest mocking (many mocked things become undefined which was really noisy internally). It would be good to test this internally and see what the ramifications are. (Though that was before we had createWarning so we can now probably exclude this warning from tests)

@facebook-github-bot
Copy link

@swaroopsm updated the pull request.

@facebook-github-bot
Copy link

@swaroopsm updated the pull request.

@gaearon
Copy link
Collaborator

gaearon commented Apr 11, 2016

Let’s also add a test for this “mixin defines a mixin” case.

@facebook-github-bot
Copy link

@swaroopsm updated the pull request.

@facebook-github-bot
Copy link

@swaroopsm updated the pull request.

'%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!

@gaearon
Copy link
Collaborator

gaearon commented Apr 12, 2016

Now the only thing holding this back is @zpao sharing his thoughts on how we want to do branches now that we use major versions. Thank you for your work!

@zpao zpao modified the milestones: 15.y.0, 15-next Jun 1, 2016
@zpao zpao modified the milestone: 15-next Jun 14, 2016
@gaearon
Copy link
Collaborator

gaearon commented Jun 26, 2016

LGTM

@gaearon gaearon merged commit 18bad06 into facebook:master Jun 26, 2016
@gaearon gaearon added this to the 15-next milestone Jun 27, 2016
gaearon added a commit that referenced this pull request Jun 27, 2016
@gaearon
Copy link
Collaborator

gaearon commented Jun 27, 2016

Note: #7126 will need to get merged right after this because this PR used Jasmine 1 assertion format.

@zpao zpao modified the milestones: 15-next, 15.3.0 Jul 13, 2016
zpao pushed a commit that referenced this pull request Jul 13, 2016
zpao pushed a commit that referenced this pull request Jul 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants