Consolidate routers #193
Consolidate routers #193
Conversation
MonicaOlejniczak
requested review from
albertogasparin,
bobrovnik and
liamqma
Co-authored-by: Liam Ma <liam.q.ma@gmail.com>
MonicaOlejniczak
force-pushed
the
improvement/consolidate-routers
branch
from
09e5023 to
8fded4d
Compare
| if (!isServerEnvironment()) { | ||
| dispatch(actions.listen()); | ||
| } | ||
| dispatch(actions.listen()); |
There was a problem hiding this comment.
No need to prevent this from being called on server?
There was a problem hiding this comment.
Ah I think this was ported incorrectly in your original changes 😂
There was a problem hiding this comment.
Should be fixed now in the latest commits
|
|
||
| bootstrapStore({ | ||
| ...bootstrapProps, | ||
| history: history || createMemoryHistory({ initialEntries: [location] }), |
There was a problem hiding this comment.
If history is now required as prop, should we mandate it here too? Wonder if this might cause problem if we pass a history v4 and consumer is using history v5
There was a problem hiding this comment.
It's a peer dependency, so it shouldn't matter. But I can change it if you like
There was a problem hiding this comment.
We said we wanna deprecate this API later on anyway, so fine to leave as is
src/controllers/router/index.tsx
Outdated
|
|
||
| return ( | ||
| return ( | ||
| <ResourceContainer isGlobal={isGlobal}> |
There was a problem hiding this comment.
Based on our earlier discussion, wonder if isGlobal here is really doing what we think. As if false, would getResourceStore() actually work??
There was a problem hiding this comment.
Well the integration tests do fail when isGlobal is false on the ResourceContainer and that is using the public API (i.e. nothing is mocked). Shall I hardcode the resource container isGlobal to true?
A continutation of #145 with the following additions:
historyprop required onRouteras it being optional previously created no reason to useMemoryRouterfor testingRouterwith a memory history passed into itcreateLocationfunctionlocationintobootstrapStoreandbootstrapStoreUniversalrespectivelylocationis always set tohistory.locationand thus goes unused in bootstrapStore and bootstrapStoreUniversalprops.locationfromRouterand preferhistory