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

getFirst current usage can result in multiple instances #879

Closed
jhsu opened this issue Jan 17, 2018 · 1 comment
Closed

getFirst current usage can result in multiple instances #879

jhsu opened this issue Jan 17, 2018 · 1 comment

Comments

@jhsu
Copy link
Collaborator

jhsu commented Jan 17, 2018

getFirst(props, types, defaultValue)

usually requires something like:

const paginator = getFirst(this.props, DataTablePanel.Paginator, <DataTablePanel.Paginator {...Paginator}/>);

which, when Paginator prop is used, constructs two instances of Paginator, one from findTypes (called by getFirst) and the one used for the defaultValue passed into getFirst.

should we be using the third argument rather than doing something like:

const paginator = getFirst(this.props, DataTablePanel.Paginator) || <DataTablePanel.Paginator {...Paginator}/>;

so that only one element is ever created?

@jhsu jhsu changed the title getFirst current usage can result in multiple instances getFirst current usage can result in multiple instances Jan 17, 2018
@ogupte
Copy link
Contributor

ogupte commented Jan 18, 2018

yes this seems like a good way to improve performance. In most cases, elements created are really minimal component wrappers with render: () => null so creating elements from these components normally don't cause performance issues, but since it's not always the case, using more short-circuit evaluation like you suggest can only make things better.

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

2 participants