search

mhkeller / layercake

graphics framework for sveltejs
https://layercake.graphics
MIT License
1.36k stars 31 forks source link

Typescript errors #218

Open rgieseke opened 2 months ago

rgieseke commented 2 months ago

Again following up after #211 (and #191), there are a bunch of Typescript errors when running npm run check.

As discussed previously (e.g. #49, #56, #117) it's tricky to get everything typed up properly with Layer Cake providing many generic layers. Still it would help if there were fewer (or no) error messages, especially when including a Layer Cake component in a Typescript based project (so ensure stuff in src/_components is error free if possible).

I'm very happy to help with cleaning things up but would be good to get your current thoughts on this, @mhkeller, if you have any.

I already tried experimenting a bit with this, but things get messy easily (e.g. more errors if one starts typing in one place.) I could also try starting with smaller PRs which are probably easier to discuss than general big picture questions.

mhkeller commented 2 months ago

From another perspective, components in src/_components are starter examples that users can include their project and those would be the least important to add types to whereas the doc site is a done thing.

There are a bunch of layer cake users who don't use typescript and so I would be worried that it would make these starter components less approachable for them if we included a bunch of type annotations that they're not used to seeing. At the same time, users who want those annotations aren't blocked on adding types to satisfy their linters etc.

If there's a middle ground I'm missing, though, feel free to start a PR with what you think is the best way to handle. Overall, wrestling with typescript isn't that appealing to me personally but if there are improvements that are easy to do then let's take a look.

rgieseke commented 2 months ago

Overall, wrestling with typescript isn't that appealing to me personally but if there are improvements that are easy to do then let's take a look.

Fully agreed! I see the benefits, but it can be quite painful, especially since there are so many ways write things. For me it's also a good way to dive a bit into Layer Cake where I normally wouldn't, so again, happy to help with that work!

Hopefully we can find a middle ground, I think it's helpful for checking where and if things break during updates.

mhkeller commented 2 months ago

I appreciate the help and enthusiasm – also helpful for me to learn things I otherwise wouldn't!

mhkeller commented 1 month ago

Dropping this cheatsheet in here for reference: https://alexharri.com/blog/jsdoc-as-an-alternative-typescript-syntax

@rgieseke I thought the PR for the axes was a good first step. Are you interested in adding any more? I can slowly make my way through, as well.

rgieseke commented 1 month ago

@rgieseke I thought the PR for the axes was a good first step. Are you interested in adding any more? I can slowly make my way through, as well.

I definitely am interested in adding more! I'll try to keep you posted when I start on something so we don't have duplicated efforts.

mhkeller commented 1 month ago

Great! What may be easiest to avoid duplicate work is to issue PRs on smaller batches of files and merge them into a branch called ts-fixes or something instead of into main to avoid having to do multiple releases and main diverging a lot from the published npm version in the mean time. Open to any other ideas, though.

edit: I made ts-fixes as a PR merge target to collect things in the meantime

mhkeller commented 1 month ago

One thing that @aslak01 brought to my attention is to not use uppercase Number, String etc types (https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html), that is one thing to do.

aslak01 commented 1 month ago

I was just doing some reading and it appears that this does not apply to JSDoc: https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html#legacy-type-synonyms

For legacy reasons, Number in JSDoc is interpreted as number, along with string and boolean.

mhkeller commented 1 month ago

What a twist! Thanks for the research!

aslak01 commented 1 month ago

I still think it's preferable to rewrite them to the lowercase variants to be clear. Even if using the capitalised variants don't cause the problems they do in regular TS, I think using the capitalised variant will contribute to unnecessary confusion.

rgieseke commented 1 month ago

I still think it's preferable to rewrite them to the lowercase variants to be clear. Even if using the capitalised variants don't cause the problems they do in regular TS, I think using the capitalised variant will contribute to unnecessary confusion.

I agree and will make a PR for that!