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

Add invariant for a mixin not to be undefined. #1305

Closed
wants to merge 1 commit into from
Closed

Add invariant for a mixin not to be undefined. #1305

wants to merge 1 commit into from

Conversation

andreypopp
Copy link
Contributor

Fixes #1302.

@@ -332,6 +332,10 @@ var RESERVED_SPEC_KEYS = {
mixins: function(ConvenienceConstructor, mixins) {
if (mixins) {
for (var i = 0; i < mixins.length; i++) {
invariant(
mixins[i] !== undefined,
Copy link
Member

Choose a reason for hiding this comment

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

We should move this check into mixSpecIntoComponent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved.

@zpao
Copy link
Member

zpao commented Apr 2, 2014

Should have said this before too, but can you rebase? Things have changed a bit so this doesn't apply

@andreypopp
Copy link
Contributor Author

Rebased.

@@ -438,6 +438,11 @@ function validateLifeCycleOnReplaceState(instance) {
*/
function mixSpecIntoComponent(ConvenienceConstructor, spec) {
invariant(
spec !== undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: you have a double space here.

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@chenglou
Copy link
Contributor

Still trying to figure out what to do with this one. Jest mocks functions so mixins: [bla()] fails unless you dontMock it. Sometimes there are too many things to dontMock. Can't always call autoMockOff() either because yeah, existing tests. We'll see.

@sebmarkbage
Copy link
Collaborator

We need to clean that up in our tests. This is a problem with jest and silently failing seems like a terrible testing strategy.

@sophiebits
Copy link
Collaborator

@sebmarkbage Anything we can do here? #2687 is another request for this.

@sebmarkbage
Copy link
Collaborator

Meh. Let's just pull it in. FB's existing tests failing isn't a reason to block this. We'll have to fix our shit.

@sophiebits
Copy link
Collaborator

@andreypopp Mind rebasing? This has since moved to src/class/ReactClass.js.

@sophiebits
Copy link
Collaborator

Closing due to lack of activity, and mixins are now a legacy feature that we'd like to avoid changing unnecessarily.

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.

Make React.createClass warn/throw if mixins contains undefined value
7 participants