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

Branch decorator breaks static class props #96

Open
slmgc opened this issue Jan 30, 2016 · 11 comments
Open

Branch decorator breaks static class props #96

slmgc opened this issue Jan 30, 2016 · 11 comments
Labels

Comments

@slmgc
Copy link

slmgc commented Jan 30, 2016

Hello!
Looks like Baobab's branch decorator breaks static class props by making them undefined.
Here's the simple test case:

import React from 'react';
import {branch} from 'baobab-react/decorators';

@branch({cursors: {}})
export default class TestClass extends React.Component {
    static testProp = true
    render() {
        console.log(TestClass.testProp); // logs 'undefined' instead of 'true'
        return null;
    }
}

"babel": "^5.8.35",
"baobab": "^2.3.3",
"baobab-react": "^1.0.1"

@Yomguithereal
Copy link
Owner

Hello @slmgc. This is strange indeed. Let me check this and fix the issue if I can.

@Yomguithereal
Copy link
Owner

Ok, I just tried your use case and was unable to reproduce the bug.

@Yomguithereal
Copy link
Owner

Maybe you have an issue with static property transpilation?

@slmgc
Copy link
Author

slmgc commented Jan 31, 2016

@Yomguithereal, I use babel@5 as a transpiler with stage-0 setting, because babel@6 doesn't have class decorators. I've checked with:

import {mixins} from 'core-decorators';
import LinkedStateMixin from 'react-addons-linked-state-mixin';

@mixins(LinkedStateMixin)
...

and it worked just fine. Which version of the babel transpiler do you use?

@Yomguithereal
Copy link
Owner

Ok, I rechecked and have the same case than you. However, the thing is the mixin decorator you are using is actually mutating the class you decorate and returns the same class. However, that's not how a decorator should work actually.

@decorate
class Something {}

// is the same as doing
const SomethingElse = decorate(Something);

// You therefore loose the reference to the initial class.

@slmgc
Copy link
Author

slmgc commented Feb 3, 2016

@Yomguithereal edited: looks like I completely forgot about the main issue: nevertheless branch decorator still breaks static props. Do you know a way how to solve this issue?

@slmgc slmgc closed this as completed Feb 3, 2016
@slmgc slmgc reopened this Feb 4, 2016
@d-oliveros
Copy link

@slmgc It's not a bug per-se, it's how higher-order components work. You're basically wrapping your original class with a new class, so when you try to access your static methods, you're actually accessing the new class which doesn't have those methods.

There's some discussion and workarounds here: acdlite/flummox#173

@slmgc
Copy link
Author

slmgc commented Jun 23, 2016

@d-oliveros I was using a decorator, not a higher order component (which is also available for baobab) and it seems to me that a decorator shouldn't have such problems.

@Yomguithereal
Copy link
Owner

There is no difference between a decorator and higher order functions normally:

// higher order
const Decorated = higherOrder(Component);

// decorated
@decorator
class Decorated

@pyrossh
Copy link

pyrossh commented May 28, 2017

Yep this breaks. I'm using react-navigation and it uses a lot of static props the moment I use a branch the props are lost and my navigation header is gone.
react-navigation/react-navigation#51 (comment)

The only way I see to solve this problem is you iterate over all the static props and assign it in the hoc (don't know if that is possible) or for now I fork the branch code to solve this. Anyways how does react-redux and react-navigation solve this.
EDIT: after looking into this they use hoist statics
https://github.com/reactjs/react-redux/blob/master/src/components/connectAdvanced.js#L1

I thought it was simple here is the code for hoist,

/**
 * Copyright 2015, Yahoo! Inc.
 * Copyrights licensed under the New BSD License. See the accompanying LICENSE file for terms.
 */
'use strict';

var REACT_STATICS = {
    childContextTypes: true,
    contextTypes: true,
    defaultProps: true,
    displayName: true,
    getDefaultProps: true,
    mixins: true,
    propTypes: true,
    type: true
};

var KNOWN_STATICS = {
    name: true,
    length: true,
    prototype: true,
    caller: true,
    arguments: true,
    arity: true
};

var isGetOwnPropertySymbolsAvailable = typeof Object.getOwnPropertySymbols === 'function';

module.exports = function hoistNonReactStatics(targetComponent, sourceComponent, customStatics) {
    if (typeof sourceComponent !== 'string') { // don't hoist over string (html) components
        var keys = Object.getOwnPropertyNames(sourceComponent);

        /* istanbul ignore else */
        if (isGetOwnPropertySymbolsAvailable) {
            keys = keys.concat(Object.getOwnPropertySymbols(sourceComponent));
        }

        for (var i = 0; i < keys.length; ++i) {
            if (!REACT_STATICS[keys[i]] && !KNOWN_STATICS[keys[i]] && (!customStatics || !customStatics[keys[i]])) {
                try {
                    targetComponent[keys[i]] = sourceComponent[keys[i]];
                } catch (error) {

                }
            }
        }
    }

    return targetComponent;
};

@Yomguithereal
Copy link
Owner

So no "clean" solution right now found by other libraries except from copying properties out of some blacklist?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants