QwikDev / qwik

Instant-loading web apps, without effort
https://qwik.dev
MIT License
20.54k stars 1.28k forks source link

[🐞] 1.7.0 routeAction$ zod$ no longer supports flattened arguments (programmatic submission with ProseMirror/JSON fields) #6663

Open colelawrence opened 3 weeks ago

colelawrence commented 3 weeks ago

Which component is affected?

Qwik Runtime

Describe the bug

A recently released change to how the zod$ types, causes my previously compiling TypeScript to break, and I don't know how to fix my code without managing the validation entirely within every routeAction$ which accepts rich-text.

I depended on Zod flattening the errors, because I am using ProseMirror to prepare JSON that is submitted into the form. In this case, I am submitting programmatically the JSON value of my rich-text editors.

But, the types are implemented in such a way that assumes that every property (recursively) in the zod$ type, could have an error attached to it.

import type { JSONObject } from "@builder.io/qwik-city";
import { z } from "@builder.io/qwik-city";
import { extractTextFromProseMirrorJson } from "~/components/extractTextFromProseMirrorJson";

export const pmObjectType = z
  .custom((data) => typeof data === "object" && data && !Array.isArray(data), {
    message: "Must be a record",
  })
  .refine((a): a is JSONObject => {
    // for testing the validation in forms
    return !/\bPDEVFAIL\b/.test(extractTextFromProseMirrorJson(a));
  }, "Must not contain invalid sequence");

export const nonEmptyPMObjectType = pmObjectType.refine(
  (a): a is JSONObject => extractTextFromProseMirrorJson(a).trim().length > 0,
  "Must not be empty",
);
export const addNarrativeAction = routeAction$(
  async (form, request) => {
    const { req, modelId } = modelRequest(request, "addNarrativeAction");
    const user = req.requireSignedIn();
...
    return {
      ok: true,
      createdNarrative: createdNarrative,
    };
  },
  zod$({
    title_pm: nonEmptyPMObjectType,
    description_pm: nonEmptyPMObjectType,
    selectedAnnotations: z
      .string()
      .array()
      .nonempty("Must select at least one annotation"),
    asDraft: z.boolean().optional(),
  }),
);

As printed in IDE:

(property) fieldErrors?: Partial<{
    [x: `title_pm.${string}`]: string;
    [x: `description_pm.${string}`]: string;
    asDraft: string;
    "selectedAnnotations[]": string[];
}> | undefined

Reproduction

https://stackblitz.com/edit/qwik-1-7-0-fielderrors-pm-obj?file=src%2Froutes%2Findex.tsx

Steps to reproduce

Use any custom zod object that can have its own validation / refinement.

System Info

System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 18.20.3 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 10.2.3 - /usr/local/bin/npm
    pnpm: 8.15.6 - /usr/local/bin/pnpm
  npmPackages:
    @builder.io/qwik: ^1.7.0 => 1.7.0 
    @builder.io/qwik-city: ^1.7.0 => 1.7.0 
    typescript: 5.3.3 => 5.3.3 
    undici: 5.28.2 => 5.28.2 
    vite: 4.5.2 => 4.5.2

Additional Information

No response

gioboa commented 3 weeks ago

Thanks for this feedback. Did you check the documentation about this? This new approach is more predictable and maintainable.

colelawrence commented 3 weeks ago

Thanks for this feedback. Did you check the documentation about this? This new approach is more predictable and maintainable.

Yes, I love this as an improvement! I think it is much clearer and a better UX for larger forms.

But, for an app like mine which uses route actions heavily with rich-text data, the 1.7.0 implementation is a breaking change with no easy fix.

I also believe the types created are incorrect when there are refinements made on the object level (see my example, and understand there is no place to find the Zod error in the existing TypeScript type).

Was my use case and reproduction understood?

gioboa commented 3 weeks ago

@tzdesign do you have any ideas for this?

tzdesign commented 3 weeks ago

@colelawrence what do you expect to output?

In the end if you don't know what you get, you can always check by object keys if there are errors. In your case you could get all errors back by going throw the Record type and find all with pm_title

// NOT TESTED ;-)
export default component$(() => {
  const valid = useValidation();
  const pmTitleErrors = useComputed$(() => {
    const errors = valid.value?.fieldErrors ?? {}
    return Object.entries(errors).reduce<string[]>((result,[key,value]) => {
      if(key.includes("pm_title") && value !== undefined){
        result.push(value)
      }
      return result
    },[])
  })
  return <>{pmTitleErrors.value.join(",")}</>;
});

While writing this feature I never thought custom will come into play. If this is to complex for you, you can also opt-out and make a custom script inside the routaction without passing validation.

tzdesign commented 3 weeks ago

@gioboa I would not call this a bug. If the type is not loose, the feature is helping you a lot with complex types as @colelawrence describes.

Having custom zod types is complex in general. I made the change, because if you use complex forms, the flatten errors are pretty bad having only the first level of keys makes it impossible to guess which error belongs to which field.