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

[Feat] Add optional initial value arguments to useNetworkStatus, useSaveData; useMemoryStatus #23

Merged
merged 7 commits into from
Nov 22, 2019

Conversation

mlampedx
Copy link
Contributor

Purpose:

I am proposing that we add an optional initial value argument to the useNetworkStatus, useSaveData, and useMemoryStatus hooks to enable the developer to provide an alternative source for this valuable data in the event that the JavaScript runtime environment/the user's browser lacks the required APIs.

This opens up the opportunity for developers to utilize relevant server-side client hints that are available. For example, in instances where the server-side client hint value is reliable and consistent with the navigator value, populating the useNetworkStatus hook on the server can enable adaptive loading on SSR, reduce re-renders, and ensure HTML is consistent between SSR and client-side hydration.

Completed:

  • Add optional initial value arguments to useNetworkStatus, useSaveData; useMemoryStatus
  • Add unsupported property to all states returned by hooks for consistency
  • Bolster test coverage to 100%
  • Update documentation to include references to the initial value arguments as well as use cases

Please let me know if you would like me to amend, revert, or clarify any of the changes that I am proposing 😄

@addyosmani
Copy link
Collaborator

Overall this implementation looks great. Thank you for working on initial value support and the corresponding tests, @mlampedx!

I would love for us to check how far this addresses @gja's original concerns in #18 and sanity check how we might want to factor in #18 (comment) or #18 (comment). I think it's completely fair if we address the rest of attribute mismatch in a separate PR, but a brief discussion before landing this is probably healthy :)

Copy link
Collaborator

@addyosmani addyosmani left a comment

Choose a reason for hiding this comment

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

Initial work LGTM with minor comments.

@mlampedx
Copy link
Contributor Author

Thank you, @addyosmani ! I have addressed your comments and followed up on the original issue thread.

@addyosmani
Copy link
Collaborator

Thank you, @mlampedx. I think there's enough value in the initial value arguments implementation here for us to land it and then make a decision on the right design for the second half of the problem over in #18. Approving and landing shortly 🚀

Copy link
Collaborator

@addyosmani addyosmani left a comment

Choose a reason for hiding this comment

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

LGTM

@addyosmani addyosmani merged commit 5483ef1 into GoogleChromeLabs:master Nov 22, 2019
@mlampedx mlampedx deleted the feat/initial-values branch November 22, 2019 01:18
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.

2 participants