fix: Improving the state serialization to URL #387
Conversation
|
This pull request is being automatically deployed with ZEIT Now (learn more). 🔍 Inspect: https://zeit.co/covid19-scenarios/covid19-scenarios/l6r7w6z3j |
dkozar
force-pushed
the
fix/improved-serialization
branch
from
9f66412 to
a8f4e00
Compare
|
I see that you added back the |
dkozar
force-pushed
the
fix/improved-serialization
branch
from
a8f4e00 to
7af25ee
Compare
| // part of the application state to be persisted in the URL | ||
| export interface PersistedState { | ||
| current: string | ||
| data: ScenarioData | ||
| } | ||
|
|
||
| // format of the application state persisted in the URL | ||
| export interface PersistedStateDto { | ||
| current: string | ||
| population: PopulationData | ||
| epidemiological: EpidemiologicalData | ||
| simulation: SimulationData | ||
| containment: TimeSeries | ||
| } |
There was a problem hiding this comment.
Since you are adding this I think it would be a good point to address the fact that current is irrelevant. What do you think we should do?
There was a problem hiding this comment.
It might be irrelevant if the user customized some params. But in general, it isn't.
I think current should still be send over the wire (via URL). Because, if i want to select one of the existing, default scenarios (say "Croatia") and I want to send the link to someone, they should see "Croatia" selected in the dropdown.
Basically, we need to persist all the state needed to reconstruct the current screen. Without persisting current you can't do that.
There was a problem hiding this comment.
Hmm it might not be irrelevant to the user who is sharing their parameters (ie. when they don't modify any params like you said) but the value is irrelevant upon deserialization. It's true that removing current will make it so we can't reconstruct an identical screen but doing so is not technically correct (the state is being initialized from the URL parameters, not the scenario defaults).
There was a problem hiding this comment.
Then we should really consider not to have the scenario as a dropdown.
We should really have a "Load existing scenario" button, which opens a popup window with a number of scenarios to load: #397
Let's do something about it! 😃 I don't think there should be any stale data. Are there any performance concerns with serialization? I'm not sure (please confirm/deny), but running the algorithm is a performance bottleneck, and not the serialization? Could we mentally separate these two? The object we're serializing is not huge. I've never seen performance problems with this object size. I think along these lines:
|
src/components/Main/Main.tsx
Outdated
| @@ -71,9 +71,10 @@ async function runSimulation( | |||
| const caseCounts: EmpiricalData = countryCaseCountData[params.population.cases] || [] | |||
| const containment: TimeSeries = intervalsToTimeSeries(params.containment.mitigationIntervals) | |||
|
|
|||
| // serializeScenarioToURL(scenarioState, params) | |||
There was a problem hiding this comment.
This is the line I mean! I don't think we can remove params since it contains the non-stale data being used.
There was a problem hiding this comment.
I understand, just commented above. To me it seems we're doing something wrong and should look at the problem from another angle.
|
Please note that this "constant serialization" to URL (upon each param change) is needed later. I'm about to address the "true history" issue: #346 So, all the changes need to immediately be pushed to browser history, and the user could use the browser Back button to get the previous set of values. But that doesn't mean that - if the user does this quickly - we'll call the algorithm each and every time and kill the browser. The call to the algorithm will stay debounced! If we're not doing it this way (i.e. if we introduce some debouncing to writing to the URL) - this opens a can of worms, in terms that the final state will not be predictable and the user won't be sure what changes did persist and which hasn't. Going Back will not be deterministic. By "constant serialization" I mean:
|
dkozar
force-pushed
the
fix/improved-serialization
branch
from
7af25ee to
fe2e999
Compare
dkozar
force-pushed
the
fix/improved-serialization
branch
from
fe2e999 to
4078eaa
Compare
|
@dkozar Thanks Danko. The restructuring is indeed useful. |
Related issues and PRs
Description
Current de/serialization is messy and I feel we're not in control. Serialization methods are communicating with browser history and window.location. This makes those methods hard to test.
Additionally, the
paramsproperty is being passed into serialization method, whereas in reality it wasn't really needed. I've removed it from the serialization methods altogether.In order to separate the concerns, I've split the serialization process into 3 layers:
I wrote unit tests for each layer. I believe this is puts us back in control.
Note: The functionality didn't change, so if you merge it there will be no visible change. But the serialization process will be easier to follow, and changes to each level will be trivial (unified type schema, JSURL)
Testing
Load the app in a browser, change parameters, reload the page and check if all the parameters are properly persisted.