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

Framework: Adding withInstanceId Higher Order Component #1140

Merged
merged 3 commits into from Jun 12, 2017

Conversation

youknowriad
Copy link
Contributor

Several components needs to generate an instanceId for HTML ids etc... In this PR I'm adding a withInstanceId Higher Order Component providing the instanceId as a prop.

@youknowriad youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label Jun 12, 2017
@youknowriad youknowriad self-assigned this Jun 12, 2017
@youknowriad youknowriad requested a review from aduth June 12, 2017 11:27
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would perhaps like to have seen tests and a README.md, but otherwise can confirm this looks and works well 👍

@nb
Copy link
Member

nb commented Jun 12, 2017

Do we need another HOC, can’t we solve this with this.id = uuid(); in the component’s constructor? This way we won’t need the state and will be simple enough to be self-contained and easy enough to online.

For the record, since c2732e7 visual editor block instances receive id as part of their props.

@youknowriad
Copy link
Contributor Author

Do we need another HOC, can’t we solve this with this.id = uuid(); in the component’s constructor?

@nb We need a constructor to do so, which forces us to use the class component syntax and I'd much rather prefer the functional components unless we explicitly need refs, state or lifecycle methods.

--
I've added a unit test and some documentation.

@nb
Copy link
Member

nb commented Jun 12, 2017

A class isn't so bad, so I’d still personally prefer less HOCs simply, because this way we need to communicate one less thing (what does this withInstanceId do and where does instanceId come from), but I don’t have strong opinion. Don’t mind me and carry on :)

@youknowriad
Copy link
Contributor Author

Another difference with the uid is that it's component-specific, maybe this difference is not that important tough.

Thanks for the discussion, I'm merging now. This PR also bootstraps the Higher-Order Components Documentation. If the concern is raised again, we could revisit.

@youknowriad youknowriad merged commit 9d57a33 into master Jun 12, 2017
@youknowriad youknowriad deleted the add/with-instance-id-hoc branch June 12, 2017 15:20
@aduth
Copy link
Member

aduth commented Jun 12, 2017

I’d still personally prefer less HOCs simply, because this way we need to communicate one less thing

I'm sympathetic to the "one less thing" argument, but higher-order components are a pretty fundamental pattern for shared component behaviors. In fact, I'd worry that shielding developers from the concept could be a disservice in, for example, understanding what something like connect is doing (creating a new modified component wrapper) vs. merely repeating the pattern (it's a thing that injects props by magic).

@nb
Copy link
Member

nb commented Jun 12, 2017

We’re on the same page about connect for sure. To me, the one-line ID generation is on the other side of the useful/indirection tradeoff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants