Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Make it possible to create subclass of Node while keeping TS happy #2772

Merged
merged 4 commits into from
Sep 16, 2022

Conversation

mattvague
Copy link
Contributor

@mattvague mattvague commented Sep 8, 2022

The Problem

Currently, MathNode is defined as a union of all of the existing subclasses of Node i.e.

  type MathNode = MathNo
    | AccessorNode
    | ArrayNode
    | AssignmentNode
    | BlockNode
   ...

This has a few consequences:

  1. Every time a node new type is added in MathJS you also have to remember to add that type to the MathNode union
  2. It's impossible (or at least as far as I can tell) to add a custom subclass of Node when using mathjs while having that node conform to the MathNode type so that TS is happy

The solution

By simply aliasing MathNode to MathNodeCommon (which all existing node types extend), typescript automatically infers that any subclasses of Node are valid MathNode types.

I did some digging and couldn't quite figure out why the MathNodeCommon type needed to exist in the first place and why it couldn't just be called MathNode, but it is what it is and I guess we can't change that now without a lot of breaking changes.

@mattvague mattvague force-pushed the types/fix-MathNode-type branch from c727d77 to 211041b Compare September 8, 2022 18:42
@mattvague mattvague marked this pull request as ready for review September 8, 2022 20:12
@josdejong
Copy link
Owner

Nice, thanks!

Indeed it would be nice to rename MathNodeCommon to MathNode but indeed let's keep it for backward compatibility 👍 .

We could probably swap the two variables, and then mark MathNodeCommon as deprecated. What do you think?

@mattvague
Copy link
Contributor Author

We could probably swap the two variables, and then mark MathNodeCommon as deprecated. What do you think?

That could work! Will probably play around with it a bit in my own app a bit to make sure it doesn't cause any issues first

@mattvague mattvague force-pushed the types/fix-MathNode-type branch from acd7047 to 8e27c03 Compare September 12, 2022 19:28
@mattvague mattvague force-pushed the types/fix-MathNode-type branch from 8e27c03 to 2294ebd Compare September 12, 2022 19:29
@mattvague
Copy link
Contributor Author

mattvague commented Sep 12, 2022

We could probably swap the two variables, and then mark MathNodeCommon as deprecated. What do you think?

Done!

That could work! Will probably play around with it a bit in my own app a bit to make sure it doesn't cause any issues first

@josdejong Tried it out in my app and seems good to go!

@josdejong
Copy link
Owner

Thanks Matt 👌

@josdejong josdejong merged commit 100f1b1 into josdejong:develop Sep 16, 2022
@josdejong
Copy link
Owner

Published now in v11.3.0

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants