anthonyshort / deku

Render interfaces using pure functions and virtual DOM
https://github.com/anthonyshort/deku/tree/master/docs
3.42k stars 131 forks source link

Children are empty - updated to latest deku and magic-virtual-element #234

Closed voronianski closed 8 years ago

voronianski commented 9 years ago

I had component working fine on 0.4.x version of deku but after upgrade to latest 0.5.4 I have ALWAYS an empty array in props.children.

Component looks like:

export default {
    propTypes: {
        resolveUrl: {
            type: 'string'
        },

        clientId: {
            type: 'string'
        }
    },

    render(component) {
        const { props } = component;

        return (
            <SoundPlayerContainer {...props}>
                <Player />
            </SoundPlayerContainer>
        );
    }
};

and SoundPlayerContainer's render function:

    //...
    render(component) {
        const { props, state, id } = component;
        const soundCloudAudio = getFromRegistry(id);

        function wrapChild (child) {
            let cloneElement = assign({}, child);
            if (cloneElement.props) {
                cloneElement.props = assign({}, cloneElement.props, state, { soundCloudAudio });
            }
            return cloneElement;
        }

        console.log(props.children);
        // [], always empty!
        return (
            <span>
                {props.children.map(wrapChild)}
            </span>
        );
    }
voronianski commented 9 years ago

@anthonyshort it looks like new version with fixes is postponed?

voronianski commented 9 years ago

@anthonyshort I've updated to latest versions and this bug is still present, I assume the problem is in https://github.com/dekujs/virtual-element if there's no progress on this then I would like to look at it probably..

anthonyshort commented 9 years ago

What version of virtual-element are you using? It should be fixed in 1.2.0

voronianski commented 9 years ago

@anthonyshort this bug is still present in 1.2.0, you can take a look on the behavior at https://github.com/soundblogs/soundplayer-widget/blob/master/src/Player.js#L117 and here - https://github.com/soundblogs/deku-soundplayer/blob/master/src/addons/SoundPlayerContainer.js#L134 (props are always empty array).

voronianski commented 9 years ago

@anthonyshort so I had some time and reproduced the case for this bug (or maybe it's by design) , you may check repo for more details - https://github.com/voronianski/deku-virtual-element-bug but I would like to share some here:

/** @jsx dom */

import dom from 'virtual-element'; // eslint-disable-line no-unused-vars
import deku from 'deku';

// container
const MyComponent = {
    render(component) {
        const { props, state } = component;

        console.log(props); // children => []

        // in real world it will wrap its' children with some additional data
        return (
            <div>
                {props.children} 
            </div>
        );
    }
};

// main app entry uses container
const Main = {
    render(component) {
        const { props } = component;

        // passing props to inner components of container
        return (
            <MyComponent {...props}>
                <div>Inner text</div>
            </MyComponent>
        );
    }
};

const App = deku.tree(
    <Main someId="12345"} />
);

deku.render(App, document.getElementById('app'));

So the main point is that <MyComponent /> receives an empty array of children because props.children of <Main /> are passed down to it, is it ok? Shouldn't deku somekind of prevent it?

anthonyshort commented 9 years ago

Thanks! I'll check this out today

anthonyshort commented 9 years ago

Ah! I see now. Because you're using spread. This is one reason why children should just belong on the component instead of on props. I can push out a fix for this so that the real children object is added last so it will work like you expect.

anthonyshort commented 8 years ago

In v2, children is separate from props for this reason, so this shouldn't be an issue anymore.