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

Add useLazyRef hook #2166

Merged
merged 2 commits into from
Sep 19, 2019
Merged

Add useLazyRef hook #2166

merged 2 commits into from
Sep 19, 2019

Conversation

AndrewMusgrave
Copy link
Member

WHY are these changes introduced?

@beefchimi and I added this while working on #2105, I broke this out of the other PR so we can take our time working on 2105 and use this hook in the meantime.

It'll allow for lazy initialization of refs, see jsdocs for a little more information

WHAT is this pull request doing?

  • Adding hook + jsdoc
  • Adding tests

🎩 checklist

@BPScott
Copy link
Member

BPScott commented Sep 19, 2019

Looks solid.

I note that quilt has a react-hooks package that contains a similar implementation: https://github.com/Shopify/quilt/blob/master/packages/react-hooks/src/hooks/lazy-ref.ts

At some point in the future if we end up adding lots of these general-purpose hooks it might be worth thinking about pushing them into quilt. I can see useIsAfterIntitalMount being a useful one all over the place.

Copy link
Contributor

@beefchimi beefchimi left a comment

Choose a reason for hiding this comment

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

it is remarkable how this is almost identical to the quilt version Ben pointed out

@AndrewMusgrave
Copy link
Member Author

Looks solid.

I note that quilt has a react-hooks package that contains a similar implementation: https://github.com/Shopify/quilt/blob/master/packages/react-hooks/src/hooks/lazy-ref.ts

At some point in the future if we end up adding lots of these general-purpose hooks it might be > worth thinking about pushing them into quilt. I can see useIsAfterIntitalMount being a useful one > all over the place.

It looks like they have a similar use mounted hook as well. It might be beneficial to share our code so we don't duplicate it. Other libraries like online store ui could make use of them as well, what do you think @beefchimi

@AndrewMusgrave AndrewMusgrave merged commit 1078041 into master Sep 19, 2019
@AndrewMusgrave AndrewMusgrave deleted the add-lazy-ref branch September 19, 2019 16:56
@beefchimi
Copy link
Contributor

@AndrewMusgrave definitely in favour of sharing more hooks!

@BPScott
Copy link
Member

BPScott commented Sep 19, 2019

It looks like they have a similar use mounted hook as well.

Their hook has cleanup so it toggles if the current component is mounted or not, while I believe yours would set a value after initial mount then never change so they're similar but subtly different

@AndrewMusgrave
Copy link
Member Author

Yup the two mounting hooks are fundamentally different, with different uses. However I added a similar one here, they're almost identically however the quilt version doesn't initialize a false value, which might be an error.

@dleroux dleroux temporarily deployed to production September 20, 2019 17:44 Inactive
@dleroux dleroux temporarily deployed to production September 23, 2019 15:28 Inactive
@dleroux dleroux temporarily deployed to production September 23, 2019 15:38 Inactive
@dleroux dleroux temporarily deployed to production September 23, 2019 16:59 Inactive
@dleroux dleroux temporarily deployed to production September 23, 2019 17:05 Inactive
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

Successfully merging this pull request may close these issues.

4 participants