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

Not using keys with JSX Metal Components produces strange behavior #107

Closed
AngeloYoun opened this issue May 9, 2016 · 6 comments
Closed
Assignees
Labels

Comments

@AngeloYoun
Copy link

There seems to be an issue when not using keys for metal-jsx. This component is supposed to be adding more inputs but instead adds duplicate copies of itself when the 'Add Filter' button is clicked.

screen shot 2016-05-09 at 11 16 32 am

Manually setting keys to the components resolves this issue.

https://github.com/AngeloYoun/liferay-plugins-ee/blob/ZOE-177/portlets/watson-portlet/docroot/js/src/components/dynamic_select_input.js

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

mairatma commented May 9, 2016

Thanks for reporting, will investigate.

@mairatma
Copy link
Contributor

I figured out why this is happening, it's actually a bug in the app code, not in the infra (though it would be nice to make it easier to debug things like this, no idea right now but I'll think about it).

This is happening because you're overriding the constructor, but you're calling super with only the first argument passed to it. Metal component constructors can receive 2 arguments, the second one specifying where the component is supposed to be rendered, or if it should skip rendering altogether. Sub components always skip rendering through the constructor, but since you're not passing the value it would receive it's rendering twice, and on the wrong position.

Also, from what I saw the reason why you're overriding the constructor is to bind the functions you'll be using inside render. That should actually be done in the created lifecycle instead, since that happens before the first render, while at the end of the constructor the component may have already been rendered. Look at this example.

By changing this for both DynamicInputSelect and SelectInput I managed to fix the problem, but you should make sure to do this for any other components, since I was just testing these ones.

As I said, I'll try to find of a way to try to make these kind of problems clearer.

@AngeloYoun
Copy link
Author

Thanks Maira, that makes sense. Will use the created to bind functions.

@mairatma
Copy link
Contributor

Sure, if all works as expected with these changes let me know so we can close this :)

@mairatma
Copy link
Contributor

I'll close this since from what you told me this is working fine now :)

Just FYI, the other issue you mentioned with keys is being tracked by mairatma/metal-incremental-dom#6.

@natecavanaugh
Copy link

@AngeloYoun FYI, a general good policy if you do need to overwrite the constructor (whether in Metal, or React) is to do:

constructor(...args) {
    super(...args);
}

And even if you need some named ones, you can always use the spread operator to pass along any you're not aware of :)

@mairatma mairatma added the bug label May 27, 2016
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

3 participants