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

expose a FormFragment component for use in complex components #129

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

scamden
Copy link
Collaborator

@scamden scamden commented Jul 15, 2023

@iway1 I think this approach might be more versatile (and better performance) than the recursive component walking I added. This lets you render whatever remaining piece of the form your component is responsible for reducing duplication but giving full control.

This is close but still WIP needs at least:

  • write a few more tests for more complex cases
  • is real recursion possible with this?
  • is it ok for FormFragment's name prop to be an intermediate name, and should it be renamed?
  • should useTsContoller's new name prop follow the same pattern and derive the prefix from context? (are there cases where we need to be able to fully override the name prefix? i think not)
  • does form fragment need to be able to support non-object schemas? (aka what if I want to make a custom array field of type number(), should i be able to reuse FormFragment to render the number field? or maybe we expose a third component which is a wrapper around renderComponentForSchemaDeep for this case)

I think these are not necessary to merge

  • fix use of getValues() to also useWatch() (this won't always rerender IIRC, maybe it's why my error messages weren't rerendering) - tried this and it didn't fix
  • might want to disable the previous recursion stuff we added or at least make the depth configurable and default to 0 (but that would be a breaking change, unless we're pretty sure no one but me is using it haha)
  • pull the schema from the field controller hook (i think this already exists with type but isn't strongly typed). i'm not sure this is important since we will have to cast it's type regardless. It does bring up a point though. That these custom components don't "know' their own schema. They only are coupled to the schema in the mapping. That's kinda fine but it would be cool to think of a way to more strongly couple them

Thoughts?

@vercel
Copy link

vercel bot commented Jul 15, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-ts-form ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 2, 2023 10:57pm

@iway1
Copy link
Owner

iway1 commented Jul 15, 2023

@scamden Cool! Checking it out.

Just curious what the main motivation for this is, was there a concrete use case you're wanting it for or just a general idea for improvement?

@scamden
Copy link
Collaborator Author

scamden commented Jul 15, 2023

I have a bunch of concrete use cases! On phone but I'll write out more later! I can add tests that show case them

renderBefore:
renderBefore && (() => renderBefore({ submit: submitFn })),
children: CustomChildrenComponent,
} as any)}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't for the life of me get it to accept these props even though I'm certain they're right..

…able jest timeout, improve test form components
@scamden
Copy link
Collaborator Author

scamden commented Jul 18, 2023

@iway1 alright added a few more test cases! This allows us to write array components without duplicating any of the Fields. It also allows us to handle recursive schemas (with a lot of frustrating Zod type writing but these should be a power user feature and at least this proves it possible).

This also provides IMO a better way to handle nested schemas than the automatic recursive thing I added previously. That solution is a little bit less work to use if you don't have any customizations but it's performance impact on TS is still significant. And also in real life I find that you end up needing to customize at different levels of nesting pretty often. We could leave it, but I'd love to discuss defaulting the recursion depth to only a single level or maybe removing the feature entirely and publishing our first breaking change.

One question I still have that I'd love your feedback on:
I needed to allow you to pass "name" for array based components so they could insert the necessary [index] into the field path. I'll comment inline where i did this, but i wasn't sure whether it's better to force the user to prefix the name themselves or automatically supply the prefix from context and let the user assume they are only providing the name for their "level" of the data (eg just the [1] instead of previousKeys[1]). I chose the former, but maybe we could make name something more clear? something like nameAtThisLevel? idk naming is hard..

@@ -73,6 +73,11 @@ export function FieldContextProvider({
);
}

export function useMaybeFieldName() {
Copy link
Collaborator Author

@scamden scamden Jul 18, 2023

Choose a reason for hiding this comment

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

let us automatically derive the name from context if there are any Fields already provided above us

src/FieldContext.tsx Outdated Show resolved Hide resolved
return (
<div data-testid={textFieldTestId}>
{label && <label>{label}</label>}
<input
data-testid={props.testId}
name={props.name}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for debugging names via screen.debug()

return (
<div data-testid={textFieldTestId}>
{label && <label>{label}</label>}
<input
data-testid={props.testId}
name={props.name}
data-testid={props.testId || defaultTextInputTestId}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added default test ids to all the inputs to easily select them in tests

</div>
);
}

function BooleanField(props: {
export function BooleanField(props: {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just made these test fields a little more robust and exported them, didn't break any previous tests

src/createSchemaForm.tsx Outdated Show resolved Hide resolved
src/createSchemaForm.tsx Outdated Show resolved Hide resolved
const { control, getValues } = useFormContext<z.TypeOf<SchemaType>>();

const namePrefix = useMaybeFieldName();
const submitter = useSubmitterContext();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i only needed this within FormFragment to supply the submitters addToCoerceUndefined methods on the field provider

) {
// Recursively check objects
// if typeNames are equal Unwrap Appropriate Types:
// optional

let { type: a, _rtf_id: idA } = unwrap(_a);
let { type: b, _rtf_id: idB } = unwrap(_b);
if (visitedTypes.has(a) && visitedTypes.has(b)) return true;
Copy link
Collaborator Author

@scamden scamden Jul 18, 2023

Choose a reason for hiding this comment

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

this allows for equality checking even in circular objects. if both a and b have been seen that means a call higher up in the stack is responsible for checking their equality. if every other property on the schema is equal for that call then they are equal so returning true here defers that check to the call higher up. (and prevents an infinite loop ofc :P )

@@ -39,4 +40,5 @@ export type RTFBaseZodType =
export type RTFSupportedZodTypes =
| RTFBaseZodType
| ZodOptional<any>
| ZodNullable<any>;
| ZodNullable<any>
| ZodLazy<any>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

supporting ZodLazy to allow the recursive schemas


export type UnwrappedRTFSupportedZodTypes = {
type: RTFSupportedZodTypes;
[HIDDEN_ID_PROPERTY]: string | null;
};

export function assertNever(x: never): never {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cleaned up the types a little here to make sure our switch is fully implemented

… move render props outside fragment and fix a bug
@@ -1,6 +1,9 @@
/** @type {import('ts-jest').JestConfigWithTsJest} */
module.exports = {
preset: "ts-jest",
testTimeout: process.env.JEST_TIMEOUT
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nice for debugging

@scamden
Copy link
Collaborator Author

scamden commented Jul 19, 2023

@iway1 ok iterated quite a bit and found a nice API for this i think! can you TAL now?

Maybe we can add a section to the docs under complex-fields as well if we like this. (Can do as a follow on as I'm hoping to make use of this feature for a project this week)

@scamden
Copy link
Collaborator Author

scamden commented Aug 1, 2023

@iway1 i'm feeling pretty solid about this contract and it's a non breaking change but I don't really want to merge without your go ahead. i could merge in our fork and test it out that way for a bit, or if you're feeling comfortable I can merge here myself. lemme know what you're thinking.

i'll probably need to merge one of the two ways by tomorrow so we can make use of this for a project.

…nt_for_use_in_complex_components

# Conflicts:
#	src/__tests__/createSchemaForm.test.tsx
#	src/createSchemaForm.tsx
@scamden
Copy link
Collaborator Author

scamden commented Nov 10, 2023

@iway1 One more ping on this. I may go ahead and merge to keep us in sync. This is working well for us.

One add on I'll probably do eventually is a wrapper around useFieldArray similar to the usTsController as we end up using that hook a lot with these.

@scamden
Copy link
Collaborator Author

scamden commented Mar 26, 2024

@iway1 you kinda done with this lib? Totally ok if so just want to set my expectations

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.

3 participants