Make hasShouldComponentUpdate detect nested functions #99

Merged
merged 2 commits into from May 8, 2015

Conversation

Projects
None yet
2 participants
@adamschoenemann
Contributor

adamschoenemann commented May 7, 2015

I haven't tested it thoroughly, but it works for me! :)

component.js
+ if (mixin.shouldComponentUpdate) {
+ return true;
+ }
+ if (mixin.mixins instanceof Array) {

This comment has been minimized.

@mikaelbr

mikaelbr May 7, 2015

Member

Could this be changed to Array.isArray? instanceof checks are generally not too good in JS.

@mikaelbr

mikaelbr May 7, 2015

Member

Could this be changed to Array.isArray? instanceof checks are generally not too good in JS.

This comment has been minimized.

@adamschoenemann

adamschoenemann May 7, 2015

Contributor

Sure, no problem

@adamschoenemann

adamschoenemann May 7, 2015

Contributor

Sure, no problem

@mikaelbr

This comment has been minimized.

Show comment
Hide comment
@mikaelbr

mikaelbr May 7, 2015

Member

Is this something that React supports as well? Never seen nested objects as mixins in React before. Could you possibly do a test verifying this as well?

Member

mikaelbr commented May 7, 2015

Is this something that React supports as well? Never seen nested objects as mixins in React before. Could you possibly do a test verifying this as well?

@adamschoenemann

This comment has been minimized.

Show comment
Hide comment
@adamschoenemann

adamschoenemann May 7, 2015

Contributor

Well, React certainly supports nested mixins. React does not attempt to set a default shouldComponentUpdate via a mixin operation, so this is not really an in issue.
As a user, I would expect the omniscient shouldComponentUpdate would only be supplied if I had not supplied one, through nested mixins or not - but maybe that's just me!

This example works fine with React:

var BaseHandler = {
    shouldComponentUpdate() {
        console.log('BaseHandler shouldComponentUpdate!');
        return true;
    }
};

var AppHandler = {
    mixins: [BaseHandler]
};

var FooHandler = React.createClass({
    mixins: [AppHandler],
    getInitialState() {
        return {foo: 1};
    },
    componentDidMount() {
        window.setTimeout(() => {
            this.setState({foo: this.state.foo + 1});
        }, 500);
    },
    render() {
        return DOM.div(null,
            this.state.foo,
            'FooHandler!'
        );
    }
});

Whereas with Omniscient, React will throw an error when attempting to set shouldComponentUpdate, as it is already defined by the nested mixin.

Contributor

adamschoenemann commented May 7, 2015

Well, React certainly supports nested mixins. React does not attempt to set a default shouldComponentUpdate via a mixin operation, so this is not really an in issue.
As a user, I would expect the omniscient shouldComponentUpdate would only be supplied if I had not supplied one, through nested mixins or not - but maybe that's just me!

This example works fine with React:

var BaseHandler = {
    shouldComponentUpdate() {
        console.log('BaseHandler shouldComponentUpdate!');
        return true;
    }
};

var AppHandler = {
    mixins: [BaseHandler]
};

var FooHandler = React.createClass({
    mixins: [AppHandler],
    getInitialState() {
        return {foo: 1};
    },
    componentDidMount() {
        window.setTimeout(() => {
            this.setState({foo: this.state.foo + 1});
        }, 500);
    },
    render() {
        return DOM.div(null,
            this.state.foo,
            'FooHandler!'
        );
    }
});

Whereas with Omniscient, React will throw an error when attempting to set shouldComponentUpdate, as it is already defined by the nested mixin.

@mikaelbr

This comment has been minimized.

Show comment
Hide comment
@mikaelbr

mikaelbr May 7, 2015

Member

I see. This would fix that, so very good! 🍰

Member

mikaelbr commented May 7, 2015

I see. This would fix that, so very good! 🍰

@adamschoenemann

This comment has been minimized.

Show comment
Hide comment
@adamschoenemann

adamschoenemann May 7, 2015

Contributor

Great! 👍

Contributor

adamschoenemann commented May 7, 2015

Great! 👍

@mikaelbr mikaelbr merged commit a1e364e into omniscientjs:master May 8, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

mikaelbr added a commit that referenced this pull request May 8, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment