Sync state with URL (Share function) #117
Conversation
There was a problem hiding this comment.
Thanks again @mreishus for this contribution!
This is a very ambitious PR, and in the comments I'm even asking for things to make it more ambitious.
That being said I wonder if it's possible to break it apart into smaller independent pieces that raise fewer design questions.
- What if we started by providing a way to paste in a JSON document describing the current request? That seems like it would easier in some ways to follow the URL if we have one encoded parameter than many.
- Parsing URL parameters into app state seems a bit fragile and adds coupling that I don't think needs to exist. Parsing URL parameters into a Redux action seems like it would not only remove that coupling, but make it easier to ensure that back-button behavior remains consistent with the app.
- We're trying to fit all the URL parameters into the different parts of the request, but it really feels like what we're doing is not mangling app state, but performing a new semantic action not currently defined in the app: something like
LOAD_REQUESTorSET_REQUEST. If we create that then I think much of the logic in this PR would disappear.
So I wonder if we could start small by creating that new action, that new semantic intent. That would let us do interesting things we currently can't, such as set a request from the console with a JSON body representing the request. That action could fit in with any other necessary work required to handle app loading, and in fact, maybe adding state-tracking for isFetchingEndpoints would be a preparatory PR that enhanced the UI while paving the way for SET_REQUEST.
Beyond this it would be nice to see consistent use of JSDoc, which is so widely supported in various IDEs and other tools. I'm curious what some of the motivations are for various optimizations here, as there's a sense in which they seem premature.
Thoughts?
| // state at launch by merging saved states and any state that's encoded in | ||
| // the URL. This is important when the URL provides partial state updates, | ||
| // which must be combined with existing state without loss of detail. | ||
| let urlParams = new URL( window.location.href ).searchParams; |
There was a problem hiding this comment.
not important, but we can also create these directly with:
const urlParams = new URLSearchParams( window.location.search );| import { getEndpoints } from '../../state/endpoints/selectors'; | ||
| import { loadEndpoints } from '../../state/endpoints/actions'; | ||
|
|
||
| /** |
There was a problem hiding this comment.
opening up with /** carries the specific meaning of starting a JSDoc block, but this is not attached to any node.
it would probably be better here to use /* and */ instead of doubling the stars, unless there's some standard I'm unaware of. if we want to emphasize the comment we can add additional visual fluff inside of it.
I couldn't find any official clarity on this, but if we want this to be a module-level docblock, maybe we should put it above the import statements?
| // They did send an endpointPath in the URL. In order to fill the entire | ||
| // endpoint state, we need to load all endpoints, then we can find a match after load. | ||
| const { dispatch } = store; | ||
| loadEndpoints( apiFromUrl, versionFromUrl )( dispatch ); |
There was a problem hiding this comment.
fetching happens automatically anyway doesn't it?
and should we be loading these also in the same way with logged-out views?
seems like isInitializing is a property that would be appropriate to handle in the core app and not in this URL middleware, for example, to make the loading display of the app better.
| // endpoint state, we need to load all endpoints, then we can find a match after load. | ||
| const { dispatch } = store; | ||
| loadEndpoints( apiFromUrl, versionFromUrl )( dispatch ); | ||
| return { isInitializing: true, endpointPathLabeledInURL }; |
There was a problem hiding this comment.
isInitializing feels a bit generic for a very specific thing here. although I think there's value in rethinking where and how this is stored, in this particular case something like isFetchingEndpoints seems clearer if debugging or trying to repurpose it.
| }; | ||
|
|
||
| // This middleware is responsible for serializing the state to the URL. | ||
| // It also handles a special case of loading endpoints and setting the selected endpoint. |
There was a problem hiding this comment.
nitpick, but if we embrace JSDoc syntax here then IDEs will use these comments to show in-place documentation when calling this function, and if we add type annotations in the @param tags then most IDEs now will highlight type errors without configuring anything and without installing TypeScript
| const result = next( action ); | ||
|
|
||
| // Serialize and upate the URL. | ||
| if ( actionsThatUpdateUrl.includes( action.type ) ) { |
There was a problem hiding this comment.
if we include the actions here directly in a switch statement it will mirror all of the other Redux reducers and middleware, and remove the indirection the array with .includes() introduces
| }; | ||
|
|
||
|
|
||
| export default serializeToUrlMiddleware; |
There was a problem hiding this comment.
I would have expected to see a listener for popstate in this middleware file but I don't see one. are we updating the app state when someone hits the back button?
|
|
||
| // Let each reducer handle its own state | ||
| const deserializedState = reducers( paramsObject, { type: DESERIALIZE_URL } ); | ||
| return deserializedState; |
There was a problem hiding this comment.
would it make sense to convert the URL parameters into Redux actions instead of app state?
if that were the case it would seem more reusable so listen for popstate events and update the app using the same mechanisms that were used to set it up originally.
alternatively, we could also create a new action that sets all parameters while clearing out the old ones. this could be used in those history events and remove the need for the deepMerge logic, which seems a bit fragile, and spread out the reducer logic into these separate places.
| endpointPathLabeledInURL: 'epu', | ||
| version: 've', | ||
| api: 'ap', | ||
| }; |
There was a problem hiding this comment.
is this necessary? if we expect big URLs, then this seems like a rather minor optimization which brings a fairly big reduction in readability. if we don't expect big URLs, do we need to shorten them?
I can imagine a |
This sounds like making a new selector and doing largely the same thing you did here, but maybe instead of serializing into a |
Obviously there is great demand for this function since there are 3 unmerged PRs with this.
I took the ideas for #107 and
Note, I did use the same centralized workaround for selecting
state.request.endpointon initial load - since that is too big to store in a URL. I couldn't figure out how to push this to one reducer.This one is pretty verbose, with lots of comments and some long variable names, but I wanted to make it easy to understand.. although sometimes I realize being too verbose can have the opposite effect.