iway1 / react-ts-form

https://react-ts-form.com
MIT License
2.01k stars 35 forks source link

Recursive generation for objects and arrays #74

Closed scamden closed 1 year ago

scamden commented 1 year ago

closes https://github.com/iway1/react-ts-form/issues/64

@iway1 I'll comment a bit inline but getting this to work turned out to be easy. Getting the types correct was HARD. I had to explicitly limit the depth of the recursion or TS got concerned that it was excessively deep or even infinite. My editor still feels a bit slow with this so maybe you can check this out and try yourself to see how it feels. if it's not feeling good maybe there's a way to optimize. It doesn't feel like TS should have to do that much work to iterate a few levels down an object and map it but 🤷

Basically this works by recursing iff there's no component found in the mapping for an object or array. The rendered field map passed to CustomChildComponent has the same structure as the data except it has JSX.Element's at the leaves where we found a mapping. Props also map to the structure of the data except that for arrays you can only pass one set of props for the whole array. I think that's likely the only possible thing because it's not an indexable tuple since the data length varies.

oh also: i'd love to use a consistent formatter. would you be cool with prettier or something similar? ✅

iway1 commented 1 year ago

Awesome!

Gonna look at this close tomorrow evening.

Do want to be really wary of typescript performance stuff, that can really mess with developer workflow in my experience and right now the ts performance is pretty good. I'm sure there's a way to get it working with good TS performance

scamden commented 1 year ago

Gonna look at this close tomorrow evening.

thank you!

Do want to be really wary of typescript performance stuff, that can really mess with developer workflow in my experience and right now the ts performance is pretty good. I'm sure there's a way to get it working with good TS performance

Agreed! let me know how it feels to you. and would love any thoughts on improving if it's slow!

i may publish this as a -beta release under zod-form assuming that we will get it working / merged somehow.. as i'd really like to use this for a feature we're building..

scamden commented 1 year ago

merged and ran prettier :)

iway1 commented 1 year ago

TBH I pulled the code and didn't notice any performance issues or anything, seemed about the same as before.

Would be good to be able to actually start testing the intellisense performance at some point like they do over in the trpc repo but personally I'd need to read up on how to do it, so eyeball test is all we've got for now I think.

So yeah, LGTM, I did some manual testing and seems like everything's working as expected.

I'll go ahead and merge and cut a release unless if you think it's good to go, just let me know. I'll do it tomorrow morning if you get back to me so you can go ahead and start using it

Also, not sure why the build command is running out of memory but I ran the tests locally and they're good. I think https://github.com/actions/runner-images/issues/70 can fix it I'll add that to the action

scamden commented 1 year ago

That's great! It could have been my gigantic repo open in a separate window slowing things down.

I think it's good to merge! Might be a few useful follow ons (couple options and maybe some docs!) but I def think this is useable as is!

I'll respond with a couple more thoughts in the morning

iway1 commented 1 year ago

Sweet yeah docs for sure. There's someone working on a standalone docsite for the past few days, I was waiting on him to create a PR with it to mess with the docs any more. Also need to document the layouts features still =D

couple options

What options were you thinking?

iway1 commented 1 year ago

i gotta fix that out of memory bug when building before I publish the new release, I'll check it out tonight

scamden commented 1 year ago

There's someone working on a standalone docsite for the past few days,

so cool! gotta a little community forming here! would it be interesting / useful to setup a free slack org for quicker collab at all? i'm happy to if that seems useful

Also need to document the layouts features still =D

true! let me know when it's ready and i'll do it!

What options were you thinking?

a few possibilities:

  1. some kind of optional wrapper component for the nested objects and / or the nested array when generated.
  2. optional renderBefore and renderAfter for object and array when generated
  3. a global wrapper component per nested schema key to wrap any generated array elements and/or object elements (could just default to div if they set to true or soemthing)
  4. option to disable the recursion if they prefer to get an error on a missing mapping (meh)
  5. this one feels less certain but potentially automatic object creation for nested schemas? i feel like it's probably better for people to have to consider this and use default values if they want it but it's a thought

i think 1, 2 and maybe 3 above are pretty important and probably pretty easy. i can take a stab soon if you agree

scamden commented 1 year ago

Would be good to be able to actually start testing the intellisense performance at some point like they do over in the trpc repo

this would be awesome. i'd like to be able to measure this for my company's repo as well!

have you worked on trpc? i just started reading the docs recently and it looks so slick

iway1 commented 1 year ago

would it be interesting / useful to setup a free slack org for quicker collab at all?

Good idea! I think I prefer discord for this though since it's normally what people use when doing internet community type stuff (Might be cool to get some users of the library in there as well). I went ahead and setup a discord channel, what's your name on there and I'll send you an invite?

have you worked on trpc? i just started reading the docs recently and it looks so slick

@scamden I tried a PR once but most of it got scrapped by the other maintainers although I think a tiny amount of my code made it through lmao. I did build this documentation / testing UI package for trpc that we use internally https://github.com/iway1/trpc-panel so I guess that's indirect contribution. (tons of zod schema parsing in that package as well)

Highly recommend TRPC, provides such a good DX if you're in a situation where you can do a monorepo with fullstack typescript. We use it on all of our apps these days.

some kind of optional wrapper component for the nested objects and / or the nested array when generated.

Is this not sort-of covered by the layouts feature? Any limitiations with:

<Form>
  {({object: {nestedFieldOne, nestedFieldTwo}})=>{
    return (
      <div>
        <div className="flex-row">
          {nestedFieldOne}
          {nestedFieldTwo}
        </div>
      </div>
    )
  }}
</Form>

Or are you thinking of having it automatically generate this type of stuff?

I do want to make sure the API doesn't get too crazy for newcomers etc, I think right now it's pretty easier to understand because conceptually it's just "your schema renders to components and you can pass them props". I guess as long as it's opt-in it can be more of "advanced users" type stuff and it won't misdirect people when picking up the library, though. Any ideas about what the API for this would look like?

Two approaches I can think of are:

<Form 
  props={{
    object: {
      container: ({childen})=>{
        return (<div>{children}</div>)
      }
    }
  }}
/>

Idk if I like that one that much and it seems repetitive to pass container per prop. Something like this maybe:

import {createObjectContainer} from '@ts-react/form'

const container = createObjectContainer(ContainerComponent);

const FormSchema = z.object({
  myObject: container({
    textField: z.string(),
    numberField: z.number(),
  })
})

this one fits more within the theme of "define your schema and it renders to a component" i think, less clutter in the props and stuff.

optional renderBefore and renderAfter for object and array when generated

I think this would be good, just having a renderBefore and renderAfter in any objects.

option to disable the recursion if they prefer to get an error on a missing mapping (meh)

I don't think this is super necessary, it's going to be obvious that they messed up by the props typings I think?

this one feels less certain but potentially automatic object creation for nested schemas? i feel like it's probably better for people to have to consider this and use default values if they want it but it's a thought

you mean autofilling values in the form state for nested objects? Like initializing with an empty object or something?

iway1 commented 1 year ago

Actually I think the new types are bricking typescript, seems to be causing an out of heap space error even if I up it significantly. If I had to guess it's the recursive calls to PropType? Pretty weird that it'd create an issue like that.

Think I need to revert the PR for now b/c the project just won't build with the new types at the moment. Super interesting though, I'm going to try and figure out if I can get it fixed.

If you want to try and mess with it maybe try to build locally after messing with the types some?

scamden commented 1 year ago

Ah dang it. I worried this might be the case. Ok I'll try to take a look and debug a bit.

scamden commented 1 year ago

Oh btw discord handle is sterling#8220

iway1 commented 1 year ago

sounds good.. That trick you did with the Prev type was super cool, but I wonder if it may be related to the issues? I really have no idea what the cause is but surely it's related to the recursion in one way or another

scamden commented 1 year ago

Ya can't take credit, I found that on an article :) the problem is without that depth limit ts errors with excessively deep recursion.. :/

iway1 commented 1 year ago

I actually had a similar problem on some other part of the codebase but handled it a little differently:

/**
 * @internal
 */
type UnwrapEffects<T extends AnyZodObject | ZodEffects<any, any>> =
  T extends AnyZodObject
    ? T
    : T extends ZodEffects<infer EffectsSchema, any>
    ? EffectsSchema extends ZodEffects<infer EffectsSchemaInner, any>
      ? EffectsSchemaInner
      : EffectsSchema
    : never;

this just unwraps two levels of zod effects. originally I had the recursion but it had the ts excessively deep error. it's less elegant but maybe better performance?

scamden commented 1 year ago

Ah ya I saw that comment. That's a pretty good idea to try at least. Maybe I'll in-line one or two levels of the recursion and see if it helps. Do you have a specific scenario that repros that I can test? (I can try to create one too)

iway1 commented 1 year ago

Repros for what? only issue with this was I think that it made it where the project wouldn't build @scamden

You should be able to test whether the issue is still happening by just trying to build locally (broke locally for me running pnpm build same as in the pipelines)

scamden commented 1 year ago

Oh gotcha! My bad I misunderstood. That's an easy repro 😀