A better choice of typing for form value
sserdyuk opened this issue · 17 comments
Hi @tanepiper
I'd like to make a case for a different choice of generic's type signature. The current choice of Record<string, unknown> type doesn't take advantage of Typescript type checking at compile time, which opens a door for hard to troubleshoot runtime issues that otherwise could be prevented. For example, this type allows me to write $validity?.typoInTheFieldName?.valid checking in non-existent field's validity.
I'd like to propose to change the generic's signature to T extends {} = Record<string, unknown>. The default value provides backward compatibility and easiness of use for people who don't care about typing, but gives us an opportunity to provide a strong type definition that takes full advantage of compile time checks.
Please see my quick illustration the difference it makes: https://stackblitz.com/edit/typescript-kxf2jj?file=index.ts
@sserdyuk Thanks I'll have a look into this, most likely this one will be a few days. I'm not happy with the typings just now, either.
@sserdyuk I've added a PR with the changes - also took some time to do a little internal refactoring and removed some internal types as they are no longer needed. I'll release it later today
Hey @sserdyuk - so running linting and it really doesn't like this.
Looking around this is not recommended:
@sserdyuk OK found what seems like a workaorund which is to create a Class then set it
This seems to work: https://stackblitz.com/edit/typescript-zvm6we?file=index.ts
Otherwise, it's the following errors:
OK! So a bit of digging came to this conclusion microsoft/TypeScript#15300
tl;dr - Cannot use Record<string, unknown> type as this forces your type to need an index signature - however Record<string, any> works fine.
It looks like most of the ESLint rules are footgun preventers, but I think this along with the stores being { [K in keyof T]: T[K] } should give pretty strong typed output for you
This gets too complicated. The { [K in keyof T]: T[K] } is basically T. So maybe it is simply Test<T = Record<string, unknown>> what we need? https://stackblitz.com/edit/typescript-achsoc?file=index.ts
After thinking a bit more, simple T would accept a number or a string where we really need an object. I am going to think about this a bit more.
What about Test<T extends object = Record<string, unknown>> https://stackblitz.com/edit/typescript-1fjspb?file=index.ts
I've tried to run the lint before suggesting it here but it appears that simply npm i isn't enough to configure the project.
Hello @sserdyuk I haven't forgotten about this. As I want to make the API more stable I want this to be as much as a final change on this as it can be, so get it right the first time. Doing T extends object causes the same error - it's a very strict ban-types rule in ESLint.
The options are:
- Go with excluding the rule in ESLint
- Simplify type layer - actually inner application doesn't need to know
Tfrom userland, so just use T at the constructor layer and enrich the store export type only with{ [K in keyof T]: T[K] } - Use
Record<string, any>
No worries. I've put together a panel of tests https://stackblitz.com/edit/typescript-3s5qye?file=index.ts where you can switch the type definitions at the top and see how they work. Based on that, I think that <T extends Record<string, any>> is perfectly fine if it makes eslint happy.
@tanepiper Just checking if I can do anything else to help you finish this one.
@sserdyuk hey - so at this point yes help would be appreciated.
I'd be happy to read and approve any PR (and please add yourself as a contributor)
@tanepiper I'd be glad to but the monorepo confuses me. Would you publish contribution guidelines like how to set things up and what to test before submitting a PR?
The repo uses Yarn instead of NPM because it needs to use its resolver, the other tool is nx.
Once installed there's a couple of commands:
npm run start formula-app --rollupConfig=apps/formula-app/rollup.config.js - this starts the small testing app I use running, it's not very pretty but it lets me test thing (I already have a branch investigating using this app for Cypress e2e testing as well but there is an issue with the internal lib resolving)
npm run build svelte-formula --skip-nx-cache --watch If you run that with the above, you get hot-reloading when you change the lib`
npm run start docs-site Runs the docosaurus site under packages/doc-site
npm run nx test svelte-formula - Runs the unit test suite
I'd also have to do the NPM publish, I don't have it automated for now - I have scripts in another repo but just haven't had time to configure it.