mathjs: TS regression: Narrowing of MathNodes by type
Describe the bug
Prior to v11.3 (not sure how far back, but certainly at least 10), MathNode would be narrowed based on its type property:
const doStuffWithMathNode(node: MathNode) => {
if (node.type === 'FunctionAssignmentNode) {
// now TS knows that node is a FunctionAssignmentNode and lets you access associated props
// like node.params
}
// back to being a wider MathNode
}
To Reproduce
About this issue
- Original URL
- State: open
- Created 2 years ago
- Comments: 23 (15 by maintainers)
Commits related to this issue
- Fix #2810: describe using typeguards instead of `.type` in history — committed to HK-SHAO/mathjs by josdejong 2 years ago
@josdejong Those exist as typeguards already! Apparently I wrote the typedefs for them…not sure why I started using
.typemore recently instead.Yes, I think that’s a good solution. Feel free to close this.
Might be worth adding a note in the 11.3.0 release notes about this, though. Also, #2772 was missing its PR link in the release notes.
Got a start here, but still some stuff to figure out: https://github.com/josdejong/mathjs/pull/2820
@mattvague gives a brief description of a use case in his intro to #2775, so you could look there, and he might have more to say on this topic.
Hmmm, it turns out it would be possible to get both the discriminated union and client extensibility, see the answer to https://stackoverflow.com/questions/46392758/widen-tagged-discriminated-union-in-typescript-in-a-different-module, at the cost that clients that wanted to do so would need to use the
declare module 'mathjs' { }syntax that I was previously unaware of. That seems like it would be the best of both worlds, with the only drawback that it would oblige clients that want to add Node types to use a somewhat unfamiliar TypeScript language facility. (We would have to provide an example of this, either in a test or in the examples directory.) Should we revert/replace #2772 with an alternate PR that sets up TypeScript to allow the extension of the MathNode union along the lines covered in the StackExchange answer? Or just leave be the new status quo with no discriminated union for nodes? Just wanted to raise the question since there is surprisingly actually an option that allows unions to be extended, so we could as ChrisopherChudzicki put it “achieve the goals of #2772 while still allowing narrowing” just by checking the .type property.Comments crossed. Sounds like the custom type guards are good enough.