Skip to content

[breaking] use devalue to (de)serialize action data #7494

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

Merged
merged 9 commits into from
Nov 15, 2022
Merged

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Nov 4, 2022

This is a breaking change for everyone who fetches the actions directly and doesn't go through use:enhance (so hopefully close to noone).

+ import { deserialize } from '$app/forms';

const response = await fetch(action, {
	method: 'POST',
	headers: {
		accept: 'application/json',
		'x-sveltekit-action': 'true'
	},
	cache: 'no-store',
	body: data,
});

- result = await response.json();
+ result = deserialize(await response.text());

Allows for Date objects and more be part of the response
Closes #7488

Draft because we first need to decide if we really want this, and if we do, I need to update the types. The public type should stay the same, but we need to adjust the usages and accomodate for the fact that data is a string until parsed.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

Allows for Date objects and more be part of the response
Closes #7488
@changeset-bot
Copy link

changeset-bot bot commented Nov 4, 2022

🦋 Changeset detected

Latest commit: bcff28c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

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

@Conduitry
Copy link
Member

Should SK expose devalue somewhere (or maybe just its parser function)? That would make sure people wouldn't be on a different version, duplicating it in their project. It would mean we could have changes in the serialization format without it being a breaking change. And it would just be one fewer dependency for people to install if they were doing this.

That said, I don't have particularly strong feelings about whether we want to be switching to devalue in the first place.

@Rich-Harris
Copy link
Member

If we go down this road then yes, definitely — probably via $app/forms since that's the only place you'd need this. We could just re-export the unflatten function from devalue (perhaps with a different name if we don't like that one?)

@dummdidumm
Copy link
Member Author

Thinking ahead, with the current system in place we can adjust the code so that it's possible for people to provide their own (de)serializer, therefore an import location that is more generic would be better. If we agree on adding something like this in the future (or reserving the right to do so), then we probably need $app/something-new because it doesn't fit in any of the existing ones.

  • $app/serde
  • $app/data
  • ?

@CalebBassham
Copy link

provide their own (de)serializer

Do you mean make it so that devalue can be replaced entirely by the user or that the user can add their own (de)serializers to devalue from sk. Either way, I can see uses for, but I don't think either are super necessary in the short-term.

@Rich-Harris
Copy link
Member

I'd vote for $app/data over $app/serde, since a lot of people aren't familiar with that term, and data is more generic and therefore future-proof.

Do you mean make it so that devalue can be replaced entirely by the user or that the user can add their own (de)serializers to devalue from sk

Would probably mean adding custom (de)serialisation to devalue. Need to establish whether there's sufficient demand first

@dummdidumm
Copy link
Member Author

I added $app/data - but I realized in the process that it's not that easy usage-wise. To deserialize the action response, you have to do const result = await result.json() and then result.data = devalue.parse(result.data) - similar things for the load response from the server. Furthermore, I'm now wondering where outside of form actions you would need to explicitly deserialize stuff - for load, that's all handled under the hood. This is not possible to encapsulate in a generic manner in a (de)serialize function. Also, when would you want to serialize something yourself? At this point you're doing custom stuff anyway.
So an export from $app/forms is probably better after all? -> switching the implementation to do that instead (we can revert that commit if we want a more general location).

@@ -91,7 +101,7 @@ export function enhance(form, submit = () => {}) {
signal: controller.signal
});

result = await response.json();
result = deserialize(await response.text());
Copy link
Member Author

Choose a reason for hiding this comment

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

We could also make it deserialize(await response.json()) but I somehow felt that passing in the raw text is better.

@Rich-Harris
Copy link
Member

@dummdidumm
Copy link
Member Author

Seems it's due to something resulting in svelte_internal getting requested two times, so there are probably two Svelte runtimes which blows everything up. The playwright trace hints at this, something about "cannot read $$ of undefined". No idea why though. Will merge master into this, let's see if that changes anything.

@dummdidumm
Copy link
Member Author

dummdidumm commented Nov 15, 2022

seems this worked, but now there's an error with the sharp package, maybe due to the bumping of the patch dependencies..? Redeploying without cache works, so it's likely a missing postinstall script running or sth.

Rerunning all jobs one more time for good measure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use devalue with form actions
4 participants