ciscoheat / sveltekit-superforms

Making SvelteKit forms a pleasure to use!
https://superforms.rocks
MIT License
2.14k stars 64 forks source link

Nested objects defaults not working/Nested Errors are flattened into an array of strings #29

Closed Bewinxed closed 1 year ago

Bewinxed commented 1 year ago

Thanks for the great module, I'm a bit new so not sure if this is a superforms issue or a zod issue but, I have the following types:

If there are validation issues on the ListingProject (which is a nested zod object), the errors show up as [error1, error2, error3] so in the form it's a bit difficult to show the error on the nested object.

Plus I assumed that superforms assumes the defaults based on the object type, for the ListingProject object, there's a couple of fields that are arrays of strings or objects, when trying to build forms that use these fields you get "cannot read attribute of undefined (reading length).

so what I do is in the nested object definition I put .default({ array1: [], array2: []} is this the best way to go about it?

Thanks for the great work!

import { z } from 'zod';

export const SocialNetworks = z.enum([
    'twitter',
    'discord',
    'telegram',
    'facebook',
    'instagram',
    'others'
]);

const SocialData = z.object({
    network: SocialNetworks,
    handle: z.string().min(1).max(100),
    members: z.number().min(0)
});

export type ListingType = z.infer<typeof Listing>;

export const ProjectTypes = ['NFT', 'DeFi', 'DAO', 'DEX', 'Games', 'Others'];

export const ListingProject = z.object({
    name: z.string().min(1).max(100),
    helloMoonCollectionId: z
        .string()
        .min(1)
        .max(50)
        // .optional()
        .default('040de757c0d2b75dcee999ddd47689c4'),
    collectionName: z.string().min(1).max(100),
    launchDate: z.date(),
    launchPrice: z.number().min(0),
    socials: z.record(SocialData).default({}),
    links: z.record(z.string().url()),
    floorPriceAtListing: z.number().min(0),
    listingsAtListing: z.number().min(0),
    volumeAtListing: z.number().min(0),
    ownersAtListing: z.number().min(0),
    supply: z.number().min(1),
    description: z.string().min(1).max(500),
    image: z.string().url(),
    website: z.string().url(),
    categories: z.array(z.enum(ProjectTypes)).default([]),
    network: z
        .enum(['Solana', 'Ethereum', 'Binance Smart Chain', 'Polygon', 'Solana', 'Others'])
        .default('Solana')
});

export const ProjectAddressTypes = z.nativeEnum({
    royalty: 'Royalty',
    treasury: 'Treasury',
    creatorAddress: 'Creator Address',
    updateAuthority: 'Update Authority',
    mintAuthority: 'Mint Authority',
    freezeAuthority: 'Freeze Authority',
    claimAuthority: 'Claim Authority',
    others: 'Others'
});

export const ProjectAddress = z.object({
    network: z.enum(['Solana', 'Ethereum', 'Binance Smart Chain', 'Polygon', 'Solana', 'Others']),
    address: z.string().min(1).max(100),
    balance: z.number().min(0).optional(),
    type: ProjectAddressTypes,
    note: z.string().min(1).max(100)
});

const ProjectFinancials = z.object({
    // item
    item: z.string().min(1).max(100),
    // item value
    value: z.number().min(0),
    // item value unit
    unit: z.enum(['SOL', 'USD', 'ETH', 'BNB', 'MATIC', 'others']).default('SOL')
});

export const TechStackItem = z.object({
    name: z.string().min(1).max(100),
    description: z.string().min(1).max(1000),
    type: z.enum(['backend', 'frontend', 'database', 'others']),
    url: z.string().url(),
    technology: z.string().min(1).max(100),
    version: z.string().min(1).max(100).default('latest')
});

export enum ListingPurpose {
    takeoverFull = 'takeoverFull',
    takeoverPartial = 'takeoverPartial',
    funding = 'funding',
    others = 'others'
}

export const PricingUnits = ['SOL', 'USD', 'ETH', 'BNB', 'MATIC'];

export const ListingPurposes = z.enum(['takeoverFull', 'takeoverPartial', 'funding', 'others']);

export const Listing = z.object({
    id: z.string().uuid(),
    user: z.string().min(1).max(50),
    created: z.date().default(() => new Date()),
    updated: z.date().nullable(),
    financials: ProjectFinancials,
    techStack: z.array(TechStackItem),
    project: ListingProject.default({ socials: {}, network: 'Solana', categories: [] }),
    headline: z.string().min(1).max(100),
    reason: z.string().min(1).max(1000),
    // purpose options ['takeover-full', 'takeover-partial', 'funding', 'others'] array
    purpose: z.array(ListingPurposes),
    partialTakeoverPercentage: z.number().min(0).max(100).optional(),
    overview: z.string().min(1).max(1000),
    addresses: z.array(ProjectAddress),
    price: z.number(),

    priceUnit: z.enum(PricingUnits).default('SOL'),
    priceIncludes: z.array(z.string().min(1).max(100)),
    type: z.enum(['auction', 'fixedPrice', 'others']).default('fixedPrice')
});
ciscoheat commented 1 year ago

Glad you like the library. :) I haven't looked at errors for nested objects, only the top level is really supported currently, but it may be remedied in version 0.6.0.

The "default value" generator may also have some cases where it can't determine what to do. Where exactly is the schema part that you have problems with?

Bewinxed commented 1 year ago

Glad you like the library. :) I haven't looked at errors for nested objects, only the top level is really supported currently, but it may be remedied in version 0.6.0.

The "default value" generator may also have some cases where it can't determine what to do. Where exactly is the schema part that you have problems with?

Thanks! mainly the ListingProject.socials, which is a nested object, so i'd add to it like ListingProject.socials.discord = {x,y,z} then you get "cannot read property of undefined" so You gotta instantiate it in the default like ListingProject.default( {socials: {} } )

Even if I set the .default() option in the nested object, it doesn't seem to work, all the .default() options in the nested object don't work.

Bewinxed commented 1 year ago

Actually after some meddling, I found that the contents of form.project (A nested object) doesn't get sent along with the form at all, so it will always return a parsing error :(

ciscoheat commented 1 year ago

Have you set options.dataType = 'json' in superForm? Without that, you cannot send any nested structures at all.

Bewinxed commented 1 year ago

Have you set options.dataType = 'json' in superForm? Without that, you cannot send any nested structures at all.

Just before you sent that, I switched the data type to json and now it works :O magic

To have more detailed error messages in nested objects, I wrote it like this:

import type { ServerLoadEvent, Actions } from '@sveltejs/kit/types';
import type { PageServerLoad } from './$types';
import { fail } from '@sveltejs/kit';
import { superValidate, type SuperValidateOptions } from 'sveltekit-superforms/server';
import {
    Listing,
    ListingProject,
    ProjectAddress,
    SocialData,
    TechStackItem
} from 'src/types/listing';
import z from 'zod';
import { error, redirect } from '@sveltejs/kit';

const listingCrudSchema = Listing.extend({
    id: Listing.shape.id.optional()
});

const listingId = () => String(Math.random()).slice(2);

export const load = (async ({ url }) => {
    // READ listing
    // For simplicity, use the id query parameter instead of a route.
    const id = url.searchParams.get('id');
    // const listing = id ? listings.find((l) => l.id === id) : null;
    const listing = listings[0];
    console.log(listings);

    if (id && !listing) throw error(404, 'Listing not found');

    // If we're editing a listing, prefill the form with the listing data.
    const form = await superValidate(listing, listingCrudSchema, { id: 'listing-form' });
    const projectForm = await superValidate(listing?.project, ListingProject, { id: 'project-form' });
    const socialsForm = await superValidate(listing?.project.socials, SocialData, {
        id: 'socials-form'
    });
    const techStackForm = await superValidate(listing?.techStack, TechStackItem, {
        id: 'tech-stack-form'
    });
    const addressesForm = await superValidate(listing?.addresses, ProjectAddress, {
        id: 'addresses-form'
    });

    // Always return { form } in load and form actions.
    return {
        form: form,
        projectForm: projectForm,
        socialsForm: socialsForm,
        techStackForm: techStackForm,
        addressesForm: addressesForm
    };
}) satisfies PageServerLoad;

export const actions = {
    default: async (event) => {
        console.log('event', event);
        const form = await superValidate(event, listingCrudSchema);
        console.log('form', form);
        const projectForm = await superValidate(form.data.project, ListingProject);
        const socialsForm = await superValidate(form.data.project.socials, SocialData);
        const techStackForm = await superValidate(form.data.techStack, TechStackItem);
        const addressesForm = await superValidate(form.data.addresses, ProjectAddress);

        if (!form.valid) {
            console.log('form invalid');
            console.log('errors', form.errors);
            console.log('project errors', projectForm.errors);
            console.log('socials errors', socialsForm.errors);
            console.log('tech stack errors', techStackForm.errors);
            console.log('addresses errors', addressesForm.errors);
            console.log('message', form.message);
            return fail(400, {
                form: form,
                projectForm: projectForm,
                socialsForm: socialsForm,
                techStackForm: techStackForm,
                addressesForm: addressesForm
            });
        }

        if (!form.data.id) {
            // CREATE user
            const listing = { ...form.data, id: listingId() };
            listings.push(listing);

            console.log('listing id', listing.id);
            throw redirect(303, '?id=' + listing.id);
        } else {
            // UPDATE user
            const listing = listings.find((u) => u.id == form.data.id);
            if (!listing) throw error(404, 'Listing not found.');

            listings[listings.indexOf(listing)] = { ...form.data, id: listing.id };

            form.message = 'Listing updated!';
            console.log(form.message);
            return {
                form: form,
                projectForm: projectForm,
                socialsForm: socialsForm,
                techStackForm: techStackForm,
                addressesForm: addressesForm
            };
        }
    }
} satisfies Actions;

// A simple listing "database"
const listings: z.infer<typeof Listing>[] = [];

Getting somewhere, form posts fine now, now i'm trying to get the errors.

then in each field of a nested object, i set it to the errors of that object, but not sure if i'm validating the other parts properly...

ciscoheat commented 1 year ago

I realize that not knowing about dataType can cause trouble like this, so in the next version, 0.6.0, there will be an error thrown if it detects nested objects and dataType is not set.

I understand that the nested errors are problematic for you, but I'll see what can be done for 0.6.0. Hopefully I can keep the current simple way of checking for errors, while also support nested ones.

Bewinxed commented 1 year ago

I realize that not knowing about dataType can cause trouble like this, so in the next version, 0.6.0, there will be an error thrown if it detects nested objects and dataType is not set.

I understand that the nested errors are problematic for you, but I'll see what can be done for 0.6.0. Hopefully I can keep the current simple way of checking for errors, while also support nested ones.

Thank you, can you help me with this: const socialsForm = await superValidate(form.data.project.socials, SocialData); i want it to validate an array of SocialData instead, but if i put z.array(SocialData) it fails with a _keyof error

TypeError: schema.keyof is not a function
    at _mapSchema (/node_modules/sveltekit-superforms/dist/server/entity.js:221:25)
    at typeInfo (/node_modules/sveltekit-superforms/dist/server/entity.js:82:12)
    at Module.entityData (/node_modules/sveltekit-superforms/dist/server/entity.js:22:23)
    at Module.superValidate (/node_modules/sveltekit-superforms/dist/server/index.js:141:23)
    at load (/home/bewinxed/Downloads/GuardianGeckoBot/templates/SolMarket/src/routes/list/+page.server.ts:37:29)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async Module.load_server_data (/node_modules/@sveltejs/kit/src/runtime/server/page/load_data.js:43:17)
    at async eval (/node_modules/@sveltejs/kit/src/runtime/server/page/index.js:166:13)
ciscoheat commented 1 year ago

Yes, this breaks the idea of superforms, which is to map an object to a (single) form. If you're using an array, it's like trying to validate multiple forms. To do that, you have to validate each object separately and set the id parameter. See this FAQ entry for more info.

ciscoheat commented 1 year ago

It should not be possible to pass an array schema to superValidate, this will be fixed in 0.6.0.

ciscoheat commented 1 year ago

It's not possible already in Typescript (should give a type error), but I'll make sure a detailed exception is thrown anyway.

teenjuna commented 1 year ago

@ciscoheat probably related: by default form returned by the superForm(data.form, { dataType: 'json' }) is empty. When I try to bind a nested value to an input field (let's say foo.bar), I instantly get an error telling me that I can't read property bar of undefined, which is understandable since there's no foo in the initial form. However, I noticed that if I just drop all the binds for the moment and submit, the form object (now returned from the action) contains all the default fields of the form, including nested fields.

So, my question is: can we receive such default object by the superForm function?

As a temporary workaround I have this under the superForm call:

$: {
    form.set({
        foo: {} // leaf fields will be created automatically
    } as z.infer<typeof schema>);
}
ciscoheat commented 1 year ago

@teenjuna I think I understand, but just to be sure: Is the form returned from superForm a completely empty object, or does it have the default values for the top-level fields of the object?

teenjuna commented 1 year ago

@ciscoheat it's just {}

Also, I just noticed that both errors and constraints can't reach nested objects. You probably already know this, just noting.

ciscoheat commented 1 year ago

That's strange, since this test works:

test.only('Deeply nested objects', async () => {
  const schema = z.object({
    id: z.number().positive(),
    user: z.object({
      name: z.string().min(2),
      posts: z.object({ subject: z.string().min(1) }).array()
    })
  });

  const data = new FormData();
  data.set('id', '123');

  const form = await superValidate(data, schema);

  expect(form.data).toStrictEqual({
    id: 123,
    user: {}
  });
});

Which means at least you'll get the top-level fields set when calling superValidate (and passing it on to the client). Would this be expected, or do you want all nested fields to have default values as well?

teenjuna commented 1 year ago

That's strange, since this test works:

But you use form.data from superValidate, while I'm talking about { form } from superForm! I mentioned that after submitting the form gets populated by the default values (inherited from schema.default() it seems). But in order to use bind:value={$form.foo.bar} on the first render, I need the fields to be initialized by superForm

or do you want all nested fields to have default values as well

What would happen I have an object with 3 level deepness? Like foo.bar.baz. If the default value will be { foo: {}}, it will erorr on foo.bar being undefined

ciscoheat commented 1 year ago

If you return { form } on the server, form.data will be used on the client when you call superForm(data.form), so when you say you get a completely empty object, I'm not sure what's the problem, since according to the test at least a top-level populated object should exist on the client. Can you post the code for when you get an empty object?

Yes, defaults for nested objects aren't supported, but should be in the next version. Unfortunately nested objects complicate the API quite a bit, so I have to find a difficult balance between simplicity and flexibility.

teenjuna commented 1 year ago

part of +page.svelte:

export let data: PageServerData;
const { form, enhance } = superForm(data.form, { dataType: 'json' });

+page.server.ts:

import type { Actions } from '@sveltejs/kit';
import { superValidate } from 'sveltekit-superforms/server';
import type { PageServerLoad } from './$types';
import schema from './schema';

export const load = (async (event) => {
    const form = await superValidate(event, schema);
    return { form };
}) satisfies PageServerLoad;

export const actions = {
    default: async (event) => {
        const form = await superValidate(event, schema);
        return { form };
    }
} satisfies Actions;

I understand that nested objects complicate things a lot. I wouldn't use them, but they make it easier to talk to backend which expects the data in this form. I hope you'll be able to find the right balance ✊

ciscoheat commented 1 year ago

This looks like it should work, so in the load function, if you console.log form.data, and also on the client for data.form, are they both {} even though the schema contains required fields?

ciscoheat commented 1 year ago

Sorry, data.form.data on the client.

teenjuna commented 1 year ago

Omg, data: PageServerData in +page.svelte doesn't contain form at all. Besides +page.server.ts I also have +page.ts (there I reuse await parent() to get data that isn't available in +page.server.ts). Looks like the +page.server.ts just gets ignored. I just started with SvelteKit, so forgive me this noob moment. Do you know if it's possible to merge them?

ciscoheat commented 1 year ago

I thought something was strange. :) Yes, this is something that's a bit hard to figure out in SvelteKit. If you want to pass along data from +page.server.ts, your +page.ts needs to look something like:

import type { PageLoad } from './$types';

export const load = (async ({ data }) => {
  return { ...data, otherData };
}) satisfies PageLoad;
ciscoheat commented 1 year ago

Default values for deeply nested objects was a simple fix, so it will be added in the next 0.5.x update. Thank you for noticing this. :)

teenjuna commented 1 year ago

your +page.ts needs to look something like

Omg, thank you for this insight! Looks like I wasn't thorough in my docs reading session... It works now!

Default values for deeply nested objects was a simple fix, so it will be added in the next 0.5.x update. Thank you for noticing this. :)

Actually, since the fix, I receive all the nested objects. Perhaps because I added .default({...}) values to my Zod schema. The only problem left: there's no way to retrieve errors and constraints for the nested fields. Everything gets flattened to the top fields (e.g. not errors: { foo: { bar: "bruh" }}, but errors: { foo: "bruh" }.

I'm yet to find a way to add a temporary workaround while it's not supported 😢

ciscoheat commented 1 year ago

Nested errors will work in 0.6.0, and maybe I can figure out a way to do the same thing for constraints as well. Those data structure must change to handle nested objects, while still keeping it simple for top level-only objects, which are the majority, so for that reason I want to keep things simple.

teenjuna commented 1 year ago

Yeah, supporting this with a good TS experience will result in some type witchcraft. Just looks at the code of felte: https://github.com/pablo-abc/felte/blob/main/packages/common/src/types.ts

ciscoheat commented 1 year ago

Ouch! I hope the Zod inference can be of help to avoid that level of complication...

ciscoheat commented 1 year ago

@teenjuna and @Bewinxed maybe you can give some feedback on the nested error structure I have in mind for 0.6.0. It's a slight rewrite of the Zod structure:

Given this schema:

const schema = z.object({
  id: z.number().positive(),
  users: z
    .object({
      name: z.string().min(2).regex(/X/),
      posts: z
        .object({ subject: z.string().min(1) })
        .array()
       .min(2)
        .optional()
    })
    .array()
});

An error structure like this can be the result:

{
  id: ['Required'],
  users: {
    '0': {
      name: ['String must contain at least 2 character(s)', 'Invalid'],
      posts: {
        '0': { subject: ['String must contain at least 1 character(s)'] },
        _errors: ['Array must contain at least 2 element(s)']
      }
    }
  }
}

This means that the top-level error handling is still the same, and if you have a nested object, you're sure to be aware of that, and have to use the other syntax. And maybe the same can be done with constraints. What do you think?

teenjuna commented 1 year ago

I think it's very good!

ciscoheat commented 1 year ago

Implemented now in v0.6, check out the announcement: https://github.com/ciscoheat/sveltekit-superforms/discussions/41