feat: Using JSURL for data serialization in links #329
Conversation
|
This pull request is being automatically deployed with ZEIT Now (learn more). 🔍 Inspect: https://zeit.co/covid19-scenarios/covid19-scenarios/q549sg6w3 |
|
I fixed the browser freezing problem and wrote unit tests around the de/serialization:
|
dkozar
changed the title
|
This is now ready for a merge. Are we going for the better URL encoding? 😄 |
| const serialize = (params: AllParams): string => { | ||
| return encodeURIComponent(JSON.stringify(params)) | ||
| } | ||
|
|
||
| const deserialize = (queryString: string): AllParams => { | ||
| try { | ||
| const obj = JSON.parse(decodeURIComponent(queryString)) | ||
|
|
||
| const simulationTimeRangeOriginal = obj.simulation.simulationTimeRange | ||
| const simulationTimeRangeConverted = { | ||
| tMin: new Date(simulationTimeRangeOriginal.tMin), | ||
| tMax: new Date(simulationTimeRangeOriginal.tMax), | ||
| } | ||
|
|
||
| // NOTE: Error in original parser. It should be: obj.containment | ||
| const containmentOriginal = obj.containment | ||
| const containmentConverted = { | ||
| ...containmentOriginal, | ||
| // NOTE: Error in original parser. It should be: containmentOriginal.reduction.map | ||
| reduction: containmentOriginal.map((c: { t: number; y: number }) => ({ | ||
| y: c.y, | ||
| t: new Date(c.t), | ||
| })), | ||
| } | ||
|
|
||
| return { | ||
| ...obj, | ||
| containment: containmentConverted, | ||
| simulation: { | ||
| ...obj.simulation, | ||
| simulationTimeRange: simulationTimeRangeConverted, | ||
| }, | ||
| } | ||
| } catch (error) { | ||
| throw new Error('encodeURIComponent: Error while parsing URL') | ||
| } | ||
| } |
There was a problem hiding this comment.
Do you think it would be possible to merge these serialize/deserialize functions and the serialize/deserialize functions in jsUrl.ts into a single deserialize function which accepts a parse function and a single serialize function which accepts a stringify function? I see that you have extra logic in the jsUrl serialize function to convert Date objects to numbers but I think that is the only difference? This probably isn't that big of deal as once I merged in my io-ts changes it will make serialization/deserialization much simpler.
There was a problem hiding this comment.
Yeah, we can massage it further afterwards. Currently there are 2 pairs of serializers because of the backward compatibility. Once we use only JSURL, this will be even simpler.
Dates are problematic, that’s why I would suggest to use timestamps starting from the DTO level.
|
I fear we have a bit of duplicated work going on with respect to this feature. Specifically, how does this square with PR #338? If these are both solving the same problem, we should begin coordinating resources. |
|
@nnoll they are not solving the same issue. Mine is solving the serialization into a string that is appended to the URL. Theirs is solving the problem one level above, and that is making the types used in the object to serialize more robust. There are conflicts, but that doesn’t mean they solve the same thing. Second one will obviously need to rebase. |
|
@dkozar Hi Danko! This is an interesting feature. Not sure if we really need this, but it is visually appealing, definitely. For example when sharing via email. Our users are non-technical mostly. Maybe they'd prefer a shorter links (compression)? I don't know. For sure, links should be shareable on social media (is there a length limitation there?) I will do something else with URLs right now #283, because this is top priority before a release in a few days. But we will definitely come back to the link format. |
Hey, JSURL gives you the balance between compression and insight what's in the data (it's human-readable).
Probably there is some limit.
Sure. Hopefully this will sort of some parts of our URL serialization, which seems very messy right now and hard to follow. I created a PR to address this, please tell me what you think: https://github.com/neherlab/covid19_scenarios/pull/387/files Maybe we could use parts of this PR. Note: This PR just improves the data flow and separates concerns (to the best of my knowledge/professional experience). It also introduces some unit tests. |
|
@ivan-aksamentov I just pulled the latest from upstream/master and it seems the de/serialization is commented out, why is that? Currently the application doesn't persist anything in the URL. |
|
@dkozar sorry, master is a bit broken now, someone have merged something wrongly. |
|
@dkozar Can you please send your email on Spectrum, we will invite you to our Slack? |
|
OK here we go. Please note that there are some issues related to strings sometimes being received from the backend (?) instead of dates.
This has a tendency to break the app and should be fixed at the source ASAP:
In order for it to work, I've wrapped some calls to Date.getTime() with try..catch. Once we mitigate the root cause, those helper functions could be removed. As far as I manually tested, persistance works as expected. There are issues with some pieces of a form (like Mitigation section), but thee doesn't seem to be related to serialization. Also, there is the 1 second debouncing issue that currently exists because of the form problem. Although the PR seems extensive, the crux is here:
So from now on, JSURL serialiser is being used. Note that this PR doesn't cover introduction of URL version. Currently the URL is like:
In later PR I'll address this so we'll have:
Unit tests are working corrrectly:
PS. Note that, in the future, if you want to change the data that will be persisted in the URL, you have to change only the |
Related issues and PRs
Description
Proposing alternative URL serialization (more read here: #326).
JSURL format: http://localhost:3000/?~(population~(ICUBeds~80~cases~%27CHE-Basel-Stadt~country~%27Switzerland~hospitalBeds~698~importsPerDay~0.1~populationServed~195000~suspectedCasesToday~10)~epidemiological~(infectiousPeriod~3~latencyTime~5~lengthHospitalStay~4~lengthICUStay~16~overflowSeverity~2~peakMonth~0~r0~2~seasonalForcing~0.2)~simulation~(simulationTimeRange~(tMax~1598918400000~tMin~1580428800000)~numberStochasticRuns~0)~containment~(~(y~1~t~1580428800000)~(y~1~t~1582483200000)~(y~0.6068708388814913~t~1584537600000)~(y~1~t~1586592000000)~(y~1~t~1588646400000)~(y~0.8250332889480693~t~1590700800000)~(y~1~t~1592755200000)~(y~0.5966444740346205~t~1594809600000)~(y~1~t~1596864000000)~(y~0.9170705725699069~t~1598918400000))~current~%27Custom)
Comparison of the old (URL encoded) and new (JSURL) format:
Note: There was an issue with Dates, because current application is passing dates to serializer. I'm suggesting to convert all the dates to Epoch before passing it to serializer.
Impacted Areas in the application
The app will continue to read the legacy URLs, however the newly created ones will be in the JSURL format.
Testing
This is a POC. I'm planning to write a thorough unit tests. There is a problem with current unit tests - it seems that a lot of tests are failing (in master branch).