[added] pluggable history implementations #169
Conversation
| /** | ||
| * Returns the current URL path in from `window.location`, including query string | ||
| */ | ||
| getWindowPath = function() { |
There was a problem hiding this comment.
Let's use a named function instead of a global here (i.e. function getWindowPath).
|
I wonder if we can get rid of the |
| * Location Handler that doesn't actually do any location handling. Instead, requests | ||
| * are sent to the server as normal. | ||
| */ | ||
| var disabledHandler = { |
There was a problem hiding this comment.
Can we name these DisabledLocation, HashLocation, etc. ?
There was a problem hiding this comment.
Sure thing. File name as well? I had them packaged in a location directory, so shall I move them up into helpers or just leave em? Will have to do some Git magic if I'm just changing the letter casing 👊
There was a problem hiding this comment.
Yeah, let's stick them in helpers.
I think git mv -f should do the trick. :)
|
if we are providing a new api to supply your own history implementation, we should change the commit message subject to |
|
(if there is a new API, I'll document it, no need for docs in this commit unless you feel like it) |
|
Sure, I'll change to Side note: it could be worth documenting the I'll make the changes recommended and rebase and squash into a single commit a little later tonight. |
|
Alright, squashed and pushed an @mjackson I just saw your comment above about removing |
| @@ -0,0 +1,10 @@ | |||
| /** | |||
There was a problem hiding this comment.
Can we leave the "s" off and name this file modules/helpers/Location.js ?
There was a problem hiding this comment.
Sure thing, I switched the filename and variable names to Location.
The vars got a little interesting in URLStore though, where there is already location (lowercase) being passed into setup() as a string, and now Location (uppercase) which is the map from name to handler. I wanted to stomp out all references to "handler", but since there was already a _location (the string), I left _locationHandler there to distinguish the difference.
Suggestions welcome if this is a little too messy.
Lazy would be not doing anything here :P Keep it up! |
|
Pushed fixes, build should be green now! |
seanadkinson
changed the title
|
how about |
seanadkinson
changed the title
|
Updated the PR name. Do I need to reword the commit message as well? Or does the release notes just read from the PR name? |
|
just the subject, talking about the refactoring is valuable in the commit message though :) |
|
Oh, I misunderstood, yeah, you need to reword the commit to change the subject, it doesn't use the pull request subject. |
|
Commit message updated. |
|
thanks for going through the trouble :) viewers of the changelog will thank you. |
|
Pushed fix for merge conflicts. Added |
|
@mjackson I'm 👍 if you are. Merge at will! |
|
I just realized we could make a |
|
@rpflorence That was the idea behind the |
|
@mjackson you ready to merge this (fix conflicts first, ofc)? |
Adds the ability to specify <Routes location={HashLocation}> instead
of just using a string. Also, use Browserify's events module instead
of event-emitter.
Also, replaces URLStore with PathStore which takes a location object
instead of using a string to identify which location it's using.
|
I added a few of my own touches, rebased, and merged manually. Thanks for your work on this @seanadkinson! :D |
This is kinda big but opens up a lot of use cases, - can build better pending navigation UI with useTransition telling app more detailed information (state, type) - replaces usePendingFormSubmit and usePendingLocation - actually aborts stale submissions/loads - fixes bugs around interrupted submissions/navigations - actions can return data now - super useful for form validation, no more screwing around with sessions - allows apps to call loaders and actions outside of navigation - manages cancellation of stale submissions and loads - reloads route data after actions - commits the freshest reloaded data along the way when there are multiple inflight allows route modules to decide if they should reload or not - after submissions - when the search params change - when the same href is navigated to other stuff - reloads route data when the same href is navigated to - does not create ghost history entries on interrupted navigation These old hooks still work, but have been deprecated for the new hooks. - useRouteData -> useLoaderData - usePendingFormSubmit -> useTransition().submission - usePendingLocation -> useTransition().location Also includes a helping of docs updates Closes #169, #151, #175, #128, #54, #208
Mostly just cut/paste code into various locations handlers. A nice side-effect is that we could use the
memoryHandlerfor rendering on the server-side I would imagine. Just need to support passing in thecurrentPathsomehow.All tests should be passing. As @mjackson warned me, I did have some trouble with the tests, but they should all be passing now, and I think the coverage isn't too bad.
This location handler API will make it easy to plug in HistoryJS support now.
closes #166