Fix multiple updates in a single render in tests #84
Conversation
This reverts commit dc46057.
| QueryParamProvider, | ||
| QueryParamContext, | ||
| QueryParamContextValue, | ||
| } from './QueryParamProvider'; |
There was a problem hiding this comment.
The changes I made modify the parameters of updateUrlQuery and the signature of QueryParamContext and the type QueryParamContextValue, which are library exports. However, these aren't documented in the README or included in any examples. Would taking these out of the library exports be considered a breaking change? They seem like internal details of the library to me.
| import { QueryParamProvider } from '../QueryParamProvider'; | ||
| import { makeMockLocation, makeMockHistory } from './helpers'; | ||
|
|
||
| // ¯\_(ツ)_/¯ |
There was a problem hiding this comment.
What should I do with these tests? I wanted to make sure the history-generating code in QueryParamProvider was still working, but I'm no longer passing history down via context.
|
Ooo cool, I wanted to get around to reworking how location/history were provided after I realized location needs to be requested at the last minute. I've made a ton of changes to the lib today (spent the day on it) that will make merging this ... difficult. But it seems like it would be worthwhile at first glance. I'll need to read through it more carefully later |
|
if you have some bandwidth to fix the merge conflicts, that'd be great too! Let me know if so, otherwise I'll probably look at it tonight or this weekend |
|
Ok going to take a look, thanks |
|
Continuing in #86 |
Resolves #62 . This is a follow-on from issue #54 and PR #61 , but the fix doesn't require
historyto havehistory.locationdefined, so now works with tests outside a browser.The approach I took was to move the refs up. Rather than each useQueryParam/s hook maintaining its own ref of the last version of the location it received, the top level provider should maintain a single location ref instead, and updates to the location should work with this shared ref. (The previous approach in #61 was like a shared ref approach, too: when it exists,
refHistory.current.locationis up-to-date everywhere in the app because there's only one history, butrefLocation.currentisn't guaranteed to be up-to-date if two different hooks call setQuery without re-rendering between calls.) I introduced a new internal-onlyLocationProvidercomponent to manage this ref and provide down a single cached updater callback.