chore: add typecheck and fix type #633
Conversation
| { "path": "./packages/conform-dom" }, | ||
| { "path": "./packages/conform-yup" }, | ||
| { "path": "./packages/conform-zod" }, | ||
| { "path": "./packages/conform-react" }, | ||
| { "path": "./packages/conform-validitystate" } |
There was a problem hiding this comment.
@edmundhung both of these work, but now if we ctrl+click the paths in vscode they goto the file
| @@ -14,7 +14,7 @@ export type SubmissionState = { | |||
| validated: Record<string, boolean>; | |||
| }; | |||
|
|
|||
| export type SubmissionContext<Value = null, FormError = string[]> = { | |||
| export type SubmissionContext<Value = unknown, FormError = string[]> = { | |||
There was a problem hiding this comment.
null here seems wrong because when running tests the value property isn't even set via parse
lifeiscontent
force-pushed
the
chore/add-typecheck
branch
from
c841225 to
82445eb
Compare
| @@ -83,7 +71,7 @@ type SubfieldMetadata< | |||
| Schema, | |||
| FormSchema extends Record<string, any>, | |||
| FormError, | |||
| > = [Schema] extends [Primitive] | |||
| > = [Schema] extends [Primitive | Date | File] | |||
There was a problem hiding this comment.
@edmundhung this seems to be the only place Primitive is being used, whats the intent for overloading the term "Primitive" here with Date and File?
maybe another term like FormValue or something along those lines might be better here
| @@ -623,7 +623,7 @@ function shouldNotify<Schema>( | |||
| return false; | |||
| } | |||
|
|
|||
| export function createFormContext< | |||
There was a problem hiding this comment.
What does this helps comparing to setting an alias on the entry file?
There was a problem hiding this comment.
you as the author know its unstable, but as the contributor I might not if I don't look at the export boundary (index) that I would read this as stable code
| @@ -18,12 +18,13 @@ | |||
| }, | |||
| "scripts": { | |||
| "build:js": "rollup -c", | |||
| "build:ts": "tsc", | |||
| "build:ts": "tsc --emitDeclarationOnly", | |||
There was a problem hiding this comment.
Why do we need to remove emitDeclarationOnly from the tsconfig?
There was a problem hiding this comment.
when running tsc with --noEmit and --emitDeclarationOnly is set in the tsconfig, tsc throws an error saying only one can be set at a time.
| get getFieldset() { | ||
| return () => | ||
| new Proxy({} as any, { | ||
| get(target, key, receiver) { | ||
| if (typeof key === 'string') { | ||
| return getFieldMetadata( | ||
| context, | ||
| subjectRef, | ||
| stateSnapshot, | ||
| name, | ||
| key, | ||
| ); | ||
| } | ||
|
|
||
| return Reflect.get(target, key, receiver); | ||
| }, | ||
| }); | ||
| }, |
There was a problem hiding this comment.
@edmundhung based off the type it says getFieldset doesn't exist so I removed it to see if tests would still pass I can revert, but we should probably update the types too
| // eslint-disable-next-line react-hooks/rules-of-hooks | ||
| const [value, setValue] = useInputValue(meta); | ||
| // eslint-disable-next-line react-hooks/rules-of-hooks |
There was a problem hiding this comment.
since the name of the function doesn't start with use it complains, I'm not sure what remix team does with thier unstable APIs to get around this but I could look into it. I'm guessing they might have lint rules turned off for things marked as unstable_
|
@edmundhung besides the revert of getFieldset, is there anything else you'd like me to address in this PR before getting it merged? |
No description provided.