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

feat: make ServerFormInfo's submittedFormData more type-safe #17

Closed
wants to merge 1 commit into from

Conversation

MichaelDeBoey
Copy link
Contributor

@MichaelDeBoey MichaelDeBoey commented Sep 24, 2022

As suggested by @kentcdodds in #7 (comment) & #7 (comment)

Comment on lines +101 to +111
export type ServerFormInfo<T extends FormValidations = {}> =
| {
submittedFormData: Record<string, string>;
inputs: Record<KeyOfString<T>, InputInfo>;
valid: false;
}
| {
submittedFormData: Record<KeyOfString<T>, string>;
inputs: Record<KeyOfString<T>, InputInfo>;
valid: true;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could even be 'simplified' (dedupe the use of inputs), but can make the type look more complex

Suggested change
export type ServerFormInfo<T extends FormValidations = {}> =
| {
submittedFormData: Record<string, string>;
inputs: Record<KeyOfString<T>, InputInfo>;
valid: false;
}
| {
submittedFormData: Record<KeyOfString<T>, string>;
inputs: Record<KeyOfString<T>, InputInfo>;
valid: true;
};
export type ServerFormInfo<T extends FormValidations = {}> = {
inputs: Record<KeyOfString<T>, InputInfo>;
} & (
| { submittedFormData: Record<string, string>; valid: false }
| { submittedFormData: Record<KeyOfString<T>, string>; valid: true }
);

Copy link
Owner

Choose a reason for hiding this comment

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

What's the use-case for not using KeyOfString<T> in the valid: false branch? Is that for if an incoming FormData includes a field not specified in formValidations, then it's automatically invalid?

Copy link
Owner

@brophdawg11 brophdawg11 Sep 29, 2022

Choose a reason for hiding this comment

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

I'm also pretty sure I need to re-think submittedFormData since it doesn't support multiple inputs of the same name at the moment 😕 - so that may affect this approach too.

It should probably just be the originally submitted FormData but that can't be directly serialized over the wire so we may need to provide some helpers. It's easy to serialize going out using new URLSearchParams(formData).toString() but unfortunately you can't do the inverse on the client (ignoring <input type="file"> for the time being).

Maybe we remove serverFormInfo.submittedFormData entirely and provide helpers for the user to serialize it back from their original formData?

export const action: ActionFunction = async ({ request }) => {
  const formData = await request.formData();
  const { inputs, valid } = await validateServerFormData(...);
    formData,
    formValidations
  );
  if (!valid) {
    return json({ 
      valid, 
      inputs,
      // Helper we provide to serialize their original formData to URLSearchParams
      formData: serializeFormData(formData),
    });
  }
};

function MyComponent() {
  let data = useActionData();
  // Helper we provide to deserialize the URLSearchParams string back to a FormData
  let submittedFormData = deserializeFormData(data.formData);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this is tricky. I kinda hate this idea, but what if every value in the submittedFormData was an array? That way we can handle multiple entries. Or maybe it's only an array if there are multiple entries... I think maybe it would be better to update the field config to handle multiple entries first and then we can think about how to handle the submitted data post-validation on the server 🤔

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah I've thought about this idea too. I don't love foregoing "using the platform" and skipping FormData but it has a number of advantages:

  • Easier serialization
  • Better typing based on your defined formValidations

If we somehow indicated in formValidations that an input would have multiple values, I wonder if we could type the string/string[] correctly? If we did this we could also maybe combine errorMessages into the main formValidations structure (need to dig into the TS side of this)...

let formValidations = {
  singleValue: {
    validations: {
      required: true,
    },
  },
  multipleValues: {
    multiple: true, // Will have multiple values
    validations: {
      required: true,
    },
    errorMessages: {
      // custom error messages
      required: () => { ... },
    }
  }
};

// Then could typings know this shape?
interface serverFormData {
  singleValue: string,
  multipleValues: string[]
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brophdawg11 I think that should be possible when using conditional types

Copy link
Owner

Choose a reason for hiding this comment

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

yeah that would be slick. I think I agree with @kentcdodds that we should solve handling multiple submitted values for the same name first and then loop back to this 👍

Choose a reason for hiding this comment

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

For inspiration, here's what they do in Zod's safeParse():

Returns an object containing either the successfully parsed data (with success: true) or a ZodError instance containing detailed information about the validation problems (with success: false).

.safeParse(data:unknown): { success: true; data: T; } | { success: false; error: ZodError; }

Choose a reason for hiding this comment

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

For those of you wanting this before it's released:

  1. Open node_modules/remix-validity-state/dist/index.d.ts and replace ServerFormInfo with:
export type ServerFormInfo<T extends FormValidations = {}> = {
    inputs: Record<KeyOfString<T>, InputInfo>;
} & (
    | { submittedFormData: Record<string, string>; valid: false }
    | { submittedFormData: Record<KeyOfString<T>, string>; valid: true }
);
  1. Install patch-package: npm i patch-package
  2. Run npx patch-package remix-validity-state.
    From StackOverflow: patch-package will then create a patches folder with a file inside, representing your changes. This file can then be commited to git, and patches can be restored later by running npx patch-package (without any arguments).
  3. Edit your package.json:
{
	"scripts": {
		"postinstall": "npx patch-package",
	}
}
  1. Subscribe to this repo's pull requests. When this change has been merged, undo all changes and update remix-validity-state: npm update remix-validity-state

@brophdawg11
Copy link
Owner

@MichaelDeBoey Apologies for the delay! The end of 2022 got super busy but finally getting back to some side projects now. I've got a PR open for my restructure now so I copied these changes into there - #26. With the new structure, I can start looking at handling multiple values and supporting string | string[] typings on submittedFormData as well

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

Successfully merging this pull request may close these issues.

4 participants