-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat: allow serialization/deserialization of custom data types (alternative API) #13149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: Rich Harris <[email protected]>
|
…:sveltejs/kit into serialize-deserialize-non-pojo-universal
This comment was marked as outdated.
This comment was marked as outdated.
preview: https://svelte-dev-git-preview-kit-13149-svelte.vercel.app/ this is an automated message |
I would prefer a version where you can tree-shake the server code:
|
We're talking about a few bytes, unless you have a realistic example to the contrary? I don't think the trade-off is a good one |
* ``` | ||
* @since 2.11.0 | ||
*/ | ||
export type Transport = Record<string, Transporter>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How much use will people really make of this specific type, since it's not helping much with type safety? Should we just omit it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think we should at least offer the defineTransport
function for type safety. We can call it something else if we don't like it but it would be nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's helping plenty — it might not be able to provide inference for the decode
parameter, but it enforces the correct shape of transport
(i.e. you can't omit either encode
or decode
for a given transporter, and you can't misspell them). And exposing Transporter
means you can add the missing type safety yourself:
export const transport: Transport = {
Foo: {
encode: (value) => ...,
decode: (data) => ...
} satisfies Transporter<Foo, [blah]>
};
We could always add defineTransport
later if we feel like we need to, but I don't think we should break with existing conventions just yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dummdidumm can we add this to 0 effort typesafe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and no - we can add satisfies Transport
for the top level, but we can't do much for the individual entries
Amazing stuff! |
Alternative to #13125, Closes #9401. Instead of separating the logic between
hooks.server.js
andhooks.client.js
, this puts both things inhooks.js
. Initially I thought the separate made more sense, but I'm not so sure. In essentially all cases, you'll need a reference to the custom type — for aninstanceof
check during serialization, and for instantiation during deserialization — so there doesn't really seem to be a benefit to keeping them in separate files.Putting them in one place is easier to understand, and means you can't end up with mismatches. It does mean that the serialization logic ends up in the client, but we're talking about a few bytes for a
value instanceof Foo && [value.blah]
function.I'm not too sure about the naming of things. Right now it looks like this:
TODO:
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.Edits