Refactor context so its has less hacks #30
Conversation
| @@ -26,15 +23,33 @@ export default function SwitcherProvider(props) { | |||
| }; | |||
| }, []); | |||
|
|
|||
| const providedMethods = useMemo(() => { | |||
There was a problem hiding this comment.
that was my initial implementation actually. but it seemed better to memo the object passed into the provider so it wouldn't cause the consumers to rerender if the provider rerendered. so i just stuck everything in one memo to make it simpler.
src/SwitcherProvider.js
Outdated
| case 'hashchange': | ||
| return hashChangeListeners.current; | ||
| default: | ||
| throw new Error('womp womp'); |
There was a problem hiding this comment.
nit: lets make this error more helpful even if it technically should never be hit.
There was a problem hiding this comment.
lol that's not a nit. thats just a good pr comment
| @@ -1,14 +1,5 @@ | |||
| import { Children } from 'react'; | |||
|
|
|||
| // http://stackoverflow.com/a/2117523 | |||
| export function generateGuid() { | |||
| load && window.addEventListener('load', handler); | ||
| pushState && window.addEventListener('popstate', handler); | ||
| hashChange && window.addEventListener('hashchange', handler); | ||
| if (usingProvider) { |
There was a problem hiding this comment.
this inversion makes the code so much better! I love it.
What do you think of ALWAYS requiring the provider? such that the second case is never even possible?
I think lots of other libraries do this. Might be worth making some of these breaking changes before the next release.
There was a problem hiding this comment.
it is kind of nice to be able to throw in one component but i think i agree it doesn't scale well if i have multiple switchers rendered in one tree. but i'm not sure how often that case would occur.
There was a problem hiding this comment.
cool, we won't do that in this PR, but maybe before the release
addresses one of the things for 4.0.0 release #29