Skip to content
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

use undefined instead of null for empty form prop #7178

Open
ivanhofer opened this issue Oct 7, 2022 · 10 comments
Open

use undefined instead of null for empty form prop #7178

ivanhofer opened this issue Oct 7, 2022 · 10 comments
Labels
breaking change feature request New feature or request forms Stuff relating to forms and form actions needs-decision Not sure if we want to do this yet, also design work needed p3-edge-case SvelteKit cannot be used in an uncommon way
Milestone

Comments

@ivanhofer
Copy link
Contributor

Describe the problem

Using the new actions feature of SvelteKit, when loading the page the form prop is set to null. In my usecase I return an errors object if some of the form fields don't pass the validation in the action.

To not having to deal with unnecessary if checks or optional chainings, I'm setting errors to an empty object when the page loads and form is still null.

<script>
	export let form
	$: ({ errors } = form || { errors: { } }
</script>

{#if errors.email}
	invalid email address
{/if}

{#if form.errors.password} <!-- Uncaught TypeError: Cannot read properties of null (reading 'errors') -->
	please enter a stronger password
{/if}

This works fine, if I only use the errors object in that file. But if I accidentially access form again, I'll get an error because form is still null.

To not having the issue above I would need to set the default value like this:

<script>
	export let form
	$: form = form || { errors: { } }
</script>

But this requires me to write additional code.

Describe the proposed solution

form should be undefined and not null to make it possible to set a fallback value like you would do in other Svelte components:

<script>
	export let form = { errors: { } }
	$: ({ errors } = form
</script>

{#if errors.email}
	invalid email address
{/if}

{#if form.errors.password} <!-- no error! -->
	please enter a stronger password
{/if}

Alternatives considered

Leave it as it is.

Importance

would make my life easier

Additional Information

No response

@benmccann benmccann added this to the 1.0 milestone Oct 7, 2022
@dummdidumm dummdidumm added feature request New feature or request p3-edge-case SvelteKit cannot be used in an uncommon way breaking change forms Stuff relating to forms and form actions labels Oct 7, 2022
@dummdidumm
Copy link
Member

dummdidumm commented Oct 10, 2022

I just tested this in the Svelte REPL, changing this to undefined wouldn't make any difference. The Svelte logic is to use the fallback value only initially, so once you set it to undefined it gets undefined. I believe there's an issue in the Svelte repo about this, but I can't find it right now.
Question is, if we want to implement this, if we do it before 1.0 so once the Svelte behavior is changed (which would be in 4.0 because it's a breaking change) we can profit from it without changing SvelteKit code.

@Tal500
Copy link
Contributor

Tal500 commented Oct 10, 2022

You may use the relatively new JS/TS language feature "?":
form?.errors?.blabla

Every "?.*" junction will return undefined if the l.h.s. is undefined or null, and the original value otherwise.

@ivanhofer
Copy link
Contributor Author

I just tested this in the Svelte REPL, changing this to undefined wouldn't make any difference. The Svelte logic is to use the fallback value only initially, so once you set it to undefined it gets undefined. I believe there's an issue in the Svelte repo about this, but I can't find it right now. Question is, if we want to implement this, if we do it before 1.0 so once the Svelte behavior is changed (which would be in 4.0 because it's a breaking change) we can profit from it without changing SvelteKit code.

Ah right, it will only set the default value on the first time. In my case I always return the same shape for error objects, so it will always be defined once I have submitted the form and the form prop gets updated. So changing it from null to undefined would support my specific use case.

For now I will continue using this approach:

<script>
	export let form
	$: form = form || { errors: { } }
</script>

Can you please link me to the Svelte issue describing the behaviour you are referring to?

@dummdidumm
Copy link
Member

Haven't found it so far, maybe the almighty @Conduitry knows, I believe he commented on that issue

@dummdidumm dummdidumm modified the milestones: 1.0, post-1.0 Oct 31, 2022
@dummdidumm
Copy link
Member

Moving this to post-1.0 (in other words, for consideration for 2.0). It's unclear by now if the behavior will be changed for Svelte 4 (although I would be in favor of it), and if it is, it would probably result in a hard-to-catch subtle change in behavior if it's possible to use SvelteKit 1.0 with Svelte 4.

@Conduitry
Copy link
Member

@dummdidumm Sorry, missed your comment from a few weeks ago. This was discussed in sveltejs/svelte#4442 and sveltejs/svelte#5673 but both of these are closed. I don't know that there is an open issue tracking this, and I'm not convinced that this is something we'd want to change anyway, even as a breaking change in Svelte 4.

@Conduitry
Copy link
Member

This behavior was also made explicit in the documentation, I'm now remembering, in sveltejs/svelte#4460.

@ivanhofer
Copy link
Contributor Author

I think this conversation is moving in a different direction.

My initial suggestion was to just use undefined instead of null to be able to set an initial default value when the page loads. With undefined we could use the same concept as for regular Svelte components.
Once a form gets submitted the developer is responsible for returning a useful value or to handle cases where nothing gets returned.

@benmccann benmccann modified the milestones: soon, 2.0 Dec 5, 2023
@benmccann benmccann added the needs-decision Not sure if we want to do this yet, also design work needed label Dec 5, 2023
@dummdidumm
Copy link
Member

I'm not sure the change is worth it if we don't change the behavior in Svelte 5 for the fallback being applied everytime the value is undefined, not just the first time. Furthermore, there's now a change in Svelte 5 which disallows changing the props of the parent if they weren't passed with bind:, and also disallows fallback values for bind: properties. This would make this pattern impossible to apply, since either the generated Root.svelte in SvelteKit passed the form with bind: to signal "yeah you can change this" and that means you can't have default values set, or we don't adjust the generated Root.svelte but that means that people can no longer manipulate properties on the form object. The latter seems like the worse option, and as such I think keeping this as null is ultimately better because it forces you to set the fallback value in a different way right away, satisfying the new rules.

@benmccann
Copy link
Member

I think we should punt on this until Svelte 5 is a bit more settled. We can revisit and make a final decision then. I'll move this to the 3.0 milestone for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change feature request New feature or request forms Stuff relating to forms and form actions needs-decision Not sure if we want to do this yet, also design work needed p3-edge-case SvelteKit cannot be used in an uncommon way
Projects
None yet
Development

No branches or pull requests

5 participants