Yomguithereal / baobab-react

React integration for Baobab.
MIT License
309 stars 38 forks source link

Branch decorator breaks static class props #96

Open slmgc opened 8 years ago

slmgc commented 8 years ago

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 commented 8 years ago

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

Yomguithereal commented 8 years ago

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

Yomguithereal commented 8 years ago

Maybe you have an issue with static property transpilation?

slmgc commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

@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?

d-oliveros commented 8 years ago

@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: https://github.com/acdlite/flummox/issues/173

slmgc commented 8 years ago

@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 commented 8 years ago

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

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

// decorated
@decorator
class Decorated
pyrossh commented 7 years ago

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. https://github.com/react-community/react-navigation/issues/51#issuecomment-298130437

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 commented 7 years ago

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