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

Class constructor is run after render method on component #116

Closed
bryceosterhaus opened this issue May 25, 2016 · 11 comments
Closed

Class constructor is run after render method on component #116

bryceosterhaus opened this issue May 25, 2016 · 11 comments
Assignees

Comments

@bryceosterhaus
Copy link
Member

In our project we were hoping to use transform-class-properties . We need this to avoid having to .bind(this) over and over. If we utilize class properties, we are able to use arrow functions in our class for those methods.

However, we realized that transform-class-properties doesn't work with metal components. While I was debugging this I realized that constructor was being called after created and render. See example below.

import Component from 'metal-jsx';

let foo = null;

class Test extends Component {
    boundFunction = () => this;

    constructor(config) {
        super(config);

        console.log(this.boundFunction); // boundFunction()
        console.log(foo.boundFunction); // boundFunction()

        console.log(this.boundFunction === foo.boundFunction); // true
    }

    created() {
        console.log(this.boundFunction); // undefined
    }

    render() {
        foo = this;

        console.log(this.boundFunction); // undefined
        console.log(foo.boundFunction); // undefined

        return {'Hello World!'};
    };
}

export default Test;

Ideally we want to be able to call this.boundFunction in render.

Does this sound right? Is there a reason why the constructor would be called after render?

@mairatma
Copy link
Contributor

Hmmm this makes sense, I hadn't realized that the transform-class-properties plugin would need to bind functions after the constructor.

The thing is that Component's constructor is the one that calls render by default. We first used to have a render function inside Component that developers would call to render the component, after constructed. We wanted to allow developers to describe the jsx contents inside a render function though, so that couldn't be the name of Component's function as well. Since most cases of component creation would always call render right after the constructor we decided to remove the function and have the constructor itself do the rendering automatically.

That's why the component has already been rendered after super is called. I see how this can be a problem for things like these though, so we probably need to create a function that has to be called manually for rendering again, it just can't be called render.

@yuchi
Copy link
Contributor

yuchi commented May 27, 2016

By the way in react this is not an issue because components don't render themselves.

@mairatma
Copy link
Contributor

Yeah, another option is to have a separate render function that can receive a component to render, like React.

@mairatma mairatma self-assigned this May 31, 2016
@mairatma
Copy link
Contributor

mairatma commented May 31, 2016

Hey guys, so I went with a compromise here that I think will work well.

I kept the default behavior for component constructors, rendering the component automatically. For simple use cases people will still be able to create and render the component with a single call.

I've created a new way to render components though, through static functions. You can now use Component.render(Test, opt_data, opt_parent) to render a component, and this function will make sure to render only after the constructor has finished running.

There's also the option of using the JSX.render function instead though, which may be best for people using jsx, since it accepts not only component constructors, but also functional components as well as plain jsx elements too, as you can see in this test example. That's a little bonus that I've been wanting to provide anyway :)

We can make this the only way to render components in the future if desired, but I preferred keeping the old way for now, so that it's not always required to import Component to render in simple cases.

So it should be possible to use transform-class-properties now, as long as you render through these functions :)

@eduardolundgren
Copy link
Contributor

Would be ideal to merge, the Component.render and JSX.render into the same method signature.

@mairatma
Copy link
Contributor

They already use the same signature, and people can just use JSX.render always. I just wanted to have Component.render too so non jsx components could also take advantage of this, while JSX.render covers cases that only exist for jsx components.

@eduardolundgren
Copy link
Contributor

What I meant is the same method and same signature, avoiding two different render methods.

@eduardolundgren
Copy link
Contributor

I would suggest to keep Component.render only that works for both.

@mairatma
Copy link
Contributor

The thing is that Component doesn't know anything about incremental dom, so it can't handle the case where you're only given an incremental dom function, since it doesn't even know about it. It can only cover the component case, but it can be very useful to be able to render something like this, which would not be possible with just Component.render.

At the same time I don't think we should keep only JSX.render, since that way you won't be able to use it with non jsx components.

@eduardolundgren
Copy link
Contributor

eduardolundgren commented Jun 1, 2016

Understood. The suggested approach could be a proxied .render(C, x, y) that infer from C (its constructor) and invokes the proper render at runtime. Need more investigation to this, although if works would be very nice to keep all in one.

@mairatma
Copy link
Contributor

mairatma commented Jun 1, 2016

We've talked more about this and though it's possible to have a single function for both these cases, it would require either a new repo or much more complex code, so we decided to continue with both functions.

The only difference is that @eduardolundgren suggested moving from JSX.render to JSXComponent.render, to follow the pattern from Component.render. That has been done already.

Again, the only difference between the two functions is that Component.render only accepts component constructors, while JSXComponent.render also accepts JSX functional components and JSX elements. Projects using JSX can always use JSXComponent.render.

@mairatma mairatma closed this as completed Jun 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants