-
Notifications
You must be signed in to change notification settings - Fork 99
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 initializeWithValue
option to useCookie
hook
#120
Conversation
Codecov Report
@@ Coverage Diff @@
## master #120 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 31 31
Lines 502 514 +12
Branches 87 91 +4
=========================================
+ Hits 502 514 +12
Continue to review full report at Codecov.
|
src/useCookie/__docs__/story.mdx
Outdated
- **options** _`IUseCookieOptions`_ _(default: {})_ - Cookie options that will be | ||
used during cookie set and delete. Has only one extra option, that relates to the hook itself: | ||
- **initializeWithValue** _`boolean`_ _(default: undefined)_ - Whether to initialize state with | ||
cookie value or initialize with `undefined` state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the default be true
, not undefined
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In code it is undefined =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... that doesn't really line up with our other docs for hooks with storeDefaultValue
though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where?
While using SSR, to avoid hydration mismatch, consider setting
initializeWithValue
option
tofalse
, this will yieldundefined
state on first render and defer value fetch till effects
execution stage.
Despite from that - maybe it is slightly confusing, as false
and undefined
both perceived as falsy values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On https://react-hookz.github.io/web/?path=/docs/side-effect-usesessionstoragevalue--example
On https://react-hookz.github.io/web/?path=/docs/side-effect-usecookie--example
It probably doesn't matter, but boolean values without a default is a little weird, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Watched the code - it defaults to true
%)
smth with my memory today 😑
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, besides the typo above
Question! Why is this hook called |
I had similar thoughts, but kept such naming to have consistency with |
Consistency with which implementations of |
LMAO, i've got confused, forgot that ive renamed it 🙃 |
BREAKING CHANGE: `useCookie` renamed to `useCookieValue` BREAKING CHANGE: `useCookieValue` default behaviour for browsers changed to fetch cookie value on state initialisation. SSR remains untouched, but requires implicit setting of `initializeWithValue` option to false, to avoid hydration mismatch.
f94a1be
to
3c3be12
Compare
@JoeDuncko i've also done renaming, re-review briefly please. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
# [2.0.0](v1.28.0...v2.0.0) (2021-06-14) ### Features * add `initializeWithValue` option to `useCookie` hook ([#120](#120)) ([17c9543](17c9543)) ### BREAKING CHANGES * `useCookie` renamed to `useCookieValue` * `useCookieValue` default behaviour for browsers changed to fetch cookie value on state initialisation. SSR remains untouched, but requires implicit setting of `initializeWithValue` option to false, to avoid hydration mismatch.
🎉 This PR is included in version 2.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
BREAKING CHANGE:
useCookie
renamed touseCookieValue
BREAKING CHANGE:
useCookieValue
default behaviour for browserschanged to fetch cookie value on state initialisation.
SSR remains untouched, but requires implicit setting of
initializeWithValue
option to false, to avoid hydration mismatch.What is the current behavior, and the steps to reproduce the issue?
Hook fetches cookie value onli on effects execution stage, thus first render always yields
undefined
state.What is the expected behavior?
Ability to enable or disable initialisation fetch.
How does this PR fix the problem?
Add
initializeWithValue
option to disable initialisation value fetch and make hook to fetch value on initialisation by default.Checklist