Add typing jq.js#12
Conversation
Only 327 remaining typing issues 2 or 3 bug where fixed along the way
|
Thanks, this looks useful. You can see I started but only put external-facing types in. I'll have to look through this closely and see about figuring out those other complex cases; I do wonder whether some parts of the code are fundamentally untypeable (or typed "any and trust me") because there are pieces of action-at-a-distance type juggling where it's trying to align with base jq semantics. I don't want to add redundant instanceof cases just for a typechecker where everything does line up by construction. The second commit seems to be almost entirely noise with some deep functional changes buried within it. I think it'd be better to have prefix jsdoc blocks rather than inline type comments strewn through the parameters. Then they'd have space for documentation too. I'm probably not going to get onto this in depth until sometime next week, but it'd be nice to get everything to an internally type-safe state if it's not too intrusive. |
|
yeah sorry for the second commit, I ctrl+shift+f (format) the code while working on the functions/mkfs I'll rework/reorder those commit for easier review and i'll ping you I tried to avoid touching the code but sometimes it's required (see:mkfn and use of typeof) to help the inference I agree that it's sometimes difficult to type some params, but explicit typing as any is acceptable There is absolutely no rush for this PR because I'll try to
|
Having a strongly typed jq.js would prevent regression and help contribution (safer review)
The current jq.js has 707 problems (most are because of weak typing, but some are more serious),
most of the weak typing issues can be fixed by :
nameType(x)withtypeoforinstanceofto unlock type inference on the used variable (keepnameTypefor message display of the type)Doing this drop the number of problems to 327
some bugs where discovered and fixed along the way:
fromjson/0definitionAlternativeOperator's.toString()use incorrect properties:this.l+this.rsh.toString()value that print the type instead of the valueto go little further (if okay for you), we could:
functionslist to anArray<{func,params}>tonumber/0does not check type and can do aNumber.parseFloat(null)) (isJQValuepossibly null ?)trimRightinstead of deprecatedtrimEndDoing this drop the number of problems to 193, we could add the missing typing annotation to drop bellow 100, but the remaining 100 issues would require a more skilled overview than me (or be ready for me asking lot of confirmation).
By the way, this lib keep impressing me. It's a hidden gem in the bland ocean of javascript ecosystem.