Skip to content

Conversation

@trueadm
Copy link
Contributor

@trueadm trueadm commented Dec 9, 2024

Enables serialization and deserialization of custom data types in the form of hooks.

Closes #9401.

@changeset-bot
Copy link

changeset-bot bot commented Dec 9, 2024

⚠️ No Changeset found

Latest commit: 66e6b2e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Rich-Harris
Copy link
Member

preview: https://svelte-dev-git-preview-kit-13125-svelte.vercel.app/

this is an automated message

@dominikg
Copy link
Member

dominikg commented Dec 9, 2024

I'd love an api where serialize and deserialize implementations for a custom type are in the same file.

They have a strict relation to one another and changing one without the other can lead to bugs. It would also simplify implementing tests that ensure serialize(deserialize(x)) equals x and deserialize(serialize(y)) equals y

Users can possibly do that in userland by importing them from such a file, but just lik param matchers i think we could provide that from a known location.

@trueadm
Copy link
Contributor Author

trueadm commented Dec 9, 2024

I'd love an api where serialize and deserialize implementations for a custom type are in the same file.

They have a strict relation to one another and changing one without the other can lead to bugs. It would also simplify implementing tests that ensure serialize(deserialize(x)) equals x and deserialize(serialize(y)) equals y

Users can possibly do that in userland by importing them from such a file, but just lik param matchers i think we could provide that from a known location.

So I tried to do something like that, but I gave up. I couldn't figure out how how we could safely take the function on the server and use it on the client, given the function references imports, and generate client code with the same logic without leaking server stuff. This is my first Kit PR, so maybe I just need someone to help me understand how that sort of thing can be done.

@dominikg
Copy link
Member

dominikg commented Dec 9, 2024

its a pair of pure functions tied together by a name, so shouldn't it work in a similar way as matchers?
maybe 3 functions if you want to add a test function too

//src/serializers/foo.js
export const is(foo: any): boolean;
export const serialize(foo: Foo): string;
export const deserialize(str: string): Foo;

due to treeshaking only one of serialize and deserialize should make it to the output of client or server.

@trueadm
Copy link
Contributor Author

trueadm commented Dec 9, 2024

its a pair of pure functions tied together by a name, so shouldn't it work in a similar way as matchers? maybe 3 functions if you want to add a test function too

//src/serializers/foo.js
export const is(foo: any): boolean;
export const serialize(foo: Foo): string;
export const deserialize(str: string): Foo;

due to treeshaking only one of serialize and deserialize should make it to the output of client or server.

The body of the functions will require importing the class though. So when it runs on the client, the import will be missing?

@dummdidumm
Copy link
Member

I'd love an api where serialize and deserialize implementations for a custom type are in the same file.

It's not done like that because currently we're not using the serialize function on the client / the deserialize function on the server.

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far! I think this doesn't handle the client side navigation case yet though, i.e. when the data loading happens via __data.json. E.g. if you start on /foo and that contains a link to /serialization-basic, so it (de)serialize the data correctly?

else fulfil(data);
const try_to_resolve = () => {
if (!deferred.has(id)) {
setTimeout(try_to_resolve, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was is necessary after all due to the dynamic imports? If so we should add a comment here stating why this is necessary.

Copy link
Member

@Rich-Harris Rich-Harris Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this feels suuuuuper hacky, surely we can find another approach?

@trueadm
Copy link
Contributor Author

trueadm commented Dec 9, 2024

@dummdidumm Thanks for the feedback. Fixed!

@teemingc teemingc added the feature / enhancement New feature or request label Dec 10, 2024
@Rich-Harris
Copy link
Member

Opened an alternative PR that puts the encoding/decoding logic in a single hook: #13149

@dummdidumm
Copy link
Member

Closing in favor of #13149

@dummdidumm dummdidumm closed this Dec 12, 2024
@dummdidumm dummdidumm deleted the serialize-deserialize-non-pojo branch December 12, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature / enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow the serialisation/deserialisation of non-POJOs

6 participants