remove superjson dependency #274
Conversation
Job #128: Bundle Size — 1019.48KiB (-1.01%).Warning Bundle contains 1 duplicate package – View duplicate packages Bundle metrics
|
| Current Job #128 |
Baseline Job #115 |
|
|---|---|---|
 Initial JS |
880.22KiB(-1.16%) |
890.58KiB |
 Initial CSS |
0B |
0B |
 Cache Invalidation |
1.5% |
0% |
| Chunks |
24 |
24 |
| Assets |
45 |
45 |
| Modules |
499(-2.54%) |
512 |
| Duplicate Modules |
30 |
30 |
 Duplicate Code |
1.31%(+1.55%) |
1.29% |
| Packages |
26(-10.34%) |
29 |
| Duplicate Packages |
1 |
1 |
Bundle size by type 1 change 1 improvement
| Current Job #128 |
Baseline Job #115 |
|
|---|---|---|
| JS |
1013.49KiB (-1.01%) |
1023.84KiB |
| Other |
5.99KiB |
5.99KiB |
View job #128 report View pr/remove-superjson branch activity View project dashboard
size-limit report 📦
|
packages/client-react-streaming/src/ManualDataTransport/ManualDataTransport.tsx
Outdated
Show resolved
Hide resolved
| * If necessary, additional deserialization steps that need to be applied on top of executing the result of `stringifyForStream` in the browser. | ||
| * Could e.g. be `SuperJSON.deserialize`. (Not needed in the case of using `serialize-javascript`) | ||
| */ | ||
| reviveFromStream?: (value: any) => any; |
There was a problem hiding this comment.
| reviveFromStream?: (value: any) => any; | |
| parseFromStream?: (value: any) => any; |
"revive" sorta makes it sound like something died and you're trying to bring it back to life. How about parse as the verb here since that's a typical term used for going from string -> structured type?
| reviveFromStream?: (value: any) => any; | |
| reviveFromStream?: (value: string) => any; |
I'm assuming this is the function called for deserialization from the result of stringifyForStream correct? Since stringifyForStream always returns a string, should the argument here always take a string?
There was a problem hiding this comment.
No, the argument is already an object. The string from stringifyForStream gets directly injected into the stream, which makes it end up in JavaScript code. (The browser essentially evals it)
So if the output of stringifyForStream is "{ \"foo\": 1 }", then reviveFromStream will be called with an object { foo: 1 }.
That's also why our own revive implementation is a no-op: nothing needs to be done. Our stringifyForStream implementation stringifies the object { foo: 1, bar: undefined } as "{ \"foo\":1, \"bar\": undefined }" (which is not valid JSON, but valid JavaScript) - and revive will then be called with the { foo: 1, bar: undefined } object again.
revive is just "adding additional life" to an already parsed/eval'ed object.
So you could have a stringifyForStream implementation that converts { foo: 1, bar: undefined } to "{ \"foo\":1, \"bar\": \"$u\" }" and revive would then be called with { foo: 1, bar: "$u" } and would be expected to swap out the "$u" with undefined.
Something like that can be useful with e.g. superjson which allows you to transport over custom types, but it's not required for our implementation or something like serialize-javascript, which just outputs valid JavaScript:
{ foo: new Date() } is just serialized as "{\"foo\":new Date(\"2024-04-09T08:49:42.044Z\")}".
There was a problem hiding this comment.
"revive" sorta makes it sound like something died and you're trying to bring it back to life.
That's kinda the point. We have a half-alive zombie object that maybe needs some last finishing touches. I thought about rehydrate, but that's very confusing as React is already taken the name.
SuperJSON uses deserialize here, which is a tad better than parse, but I'm still not super happy with it.
Realistically: I don't expect that a few people will ever use this.
There was a problem hiding this comment.
I just shared this yak shave with my RKT co-maintainers and @EskiMojo14 pointed out that JSON.parse has a reviver function argument. I probably took the name from there? 🤔
| ).replaceAll(JSON.stringify(undefinedPlaceholder), "undefined"); | ||
| } | ||
|
|
||
| export function revive(value: any): any { |
There was a problem hiding this comment.
| export function revive(value: any): any { | |
| export function parse(value: any): any { |
How about parse?
| export function revive(value: any): any { | |
| export function revive(value: string): any { |
Should this take a string since that is what is serialized?
packages/client-react-streaming/src/ManualDataTransport/serialization.ts
Outdated
Show resolved
Hide resolved
|
|
||
| const stringified = JSON.stringify(value); | ||
| while (stringified.includes(JSON.stringify(undefinedPlaceholder))) { | ||
| undefinedPlaceholder = "$" + undefinedPlaceholder; |
| } | ||
|
|
||
| export function revive(value: any): any { | ||
| return value; |
There was a problem hiding this comment.
I see this revive function replaced the SuperJSON.deserialize call.
I'm curious about the default here to just return the value unchanged instead of trying to parse it. Should this do a JSON.parse or equivalent of some kind?
There was a problem hiding this comment.
Before, we had SuperJSON.stringify and SuperJSON.deserialize here.
Quoting their docs:
- serialize: Serializes any JavaScript value into a JSON-compatible object.
- deserialize: Deserializes the output of Superjson back into your original value.
- stringify: Serializes and then stringifies your JavaScript value.
- parse: Parses and then deserializes the JSON string returned by stringify.
=>
- in one direction, we need to serialize and stringify
- in the other direction, parsing is done by the browser and we only need to deserialize (if there was any additionaly serialization done in the first place)
| ] as const) { | ||
| test(JSON.stringify(data), () => { | ||
| const stringified = stringify(data); | ||
| const result = revive(eval(`(${htmlEscapeJsonString(stringified)})`)); |
There was a problem hiding this comment.
I notice you're not using eval in the actual usage of revive. Should this try and test revive as well in the way that it is used in the other parts of the codebase? Perhaps I'm missing something, but this feels a bit like a false positive since the default revive just returns the value unchanged (which means eval is doing all the work here).
There was a problem hiding this comment.
That would probably mean writing it into a <script> tag in a file, starting a browser and then pointing it to that file ;)
=> Essentially, what the browser is doing is an eval, and I don't think we really have a better way of testing it.
…ization.ts Co-authored-by: Jerel Miller <jerelmiller@gmail.com>
|
I tried to explain this better. I'm really sorry for the 🤯 - I had a ton of difficulties wrapping my mind around what needs to be done here and what's already implicitly happening, too, but I didn't really know how to explain it better (without half a page of text, at least). |
closes #209
This is a big bundle size reduction for the library itself (see below, -44.93% on what we ship to the browser with the Next.js package), but beyond that it also cuts down a lot on data transported by the library, as SuperJSON adds a lot of extra bulk in their serialization format that we just don't need.
That said, some of our users might have non-serializable values in the cache and also want that transported over, so I'm leaving an escape hatch:
stringifyandrevivecan be configured.