feat: serialize platform state to the url #245
Conversation
fspoettel
changed the title
|
@breezykermo I would appreciate a high-level review of the PR in the current state - does this approach make sense to you? There a couple of The PR is missing support for |
|
Sure thing, I'll review this afternoon (EST) |
There was a problem hiding this comment.
@fspoettel this is a great start! I've made some comments inline below. My main comment is about the internal DSL you are using to project values serialized in URL query parameters to the state. Let me know if my comments are unclear or need further explanation
There was a problem hiding this comment.
Thanks again for this contribution @fspoettel, I'm excited to see this in action! I have put some preliminary comments inline, and I will look to test this feature with a few of our configurations at some point this week to test it out. Very excited to see it in action.
| * Schema specifies how redux state maps to the url and vice versa. | ||
| * `trigger`: action that triggers a call to `dehydrate()` | ||
| * `type`: type of the mapped URL property | ||
| * `dehydrate()`: maps redux state to url state. | ||
| * `rehydrate()`: | ||
| * maps url state to redux state. | ||
| * !for performance reasons, this function works with a mutable ref to `state`! |
There was a problem hiding this comment.
Very clear explanation, thanks!
There was a problem hiding this comment.
is there any option to not use mutable ref to state, given that JS is pass by reference for objects and arrays? I just ask as I think that this final line might be a bit confusing / could use further explanation.
There was a problem hiding this comment.
We could return a change object here and deepMerge it in the reducer. This is more overhead though. Your call.
| dehydrate(state) { | ||
| return getSelected(state).map(({ id }) => id); | ||
| }, | ||
| // TODO: determine time range if `range` not set. |
There was a problem hiding this comment.
Is this TODO necessary before we can merge?
There was a problem hiding this comment.
No, I recorded smaller inprovements with TODO so that we can merge sooner.
| dehydrate(state) { | ||
| return selectActiveColorSets(state); | ||
| }, | ||
| // TODO: color parent if all children checked. |
There was a problem hiding this comment.
Likewise here: should this be completed?
* refactor plugin file structure * add support for primitives to URLState
* extract rehydration logic to plugin directory * serialize filters as leaf ids rather than full paths
|
did you have a chance to test this yet @breezykermo? |
|
@fspoettel my apologies for the long delay- this looks really great! As I'm working on a small project with timemap in the coming days, I am going to merge this and incorporate it as part of the supported feature set going forward. If there are any issues I'll hotfix in that project. |
closes #76
This is a proof-of-concept for serializing platform state to the URL.
It's based on three components:
URLStateclass that deals with serialization to the URL.I considered a few other approaches but ultimately settled on this for a few reasons:
routerlibrary would be a huge change and likely break downstream maps. this implementation is comparatively isolated.appstate is too big and complex to write it to the URL in its full form. we need to map it to a simpler representation.In addition, I made the following design decisions:
UrlStatefrom the middleware allows for behavior changes if we discover that we'd rather serialize state manually (e.g. with a share button).history.replaceStateis used istd. ofpushState. When interacting with the map, a lot of different events might be accessed, rendering the browser back button useless if the platform is linked to.replaceStateseems preferable.I would appreciate early feedback.