Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

WIP Bring back discriminated union for MathNode #2820

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

mattvague
Copy link
Contributor

No description provided.

@josdejong
Copy link
Owner

Thanks for your PR Matt, do you already want feedback on the draft?

@mattvague mattvague marked this pull request as ready for review November 7, 2022 17:40
@mattvague
Copy link
Contributor Author

Thanks for your PR Matt, do you already want feedback on the draft?

No problem. Yes I would like feedback, but with the caveat that I need to have another look over it myself as well.

@josdejong
Copy link
Owner

I just reviewed the PR, it looks good to me and well tested 👌, thanks!

Thoughts:

  1. Do we need to describe how to implement a new Node class in TS in the documentation?
  2. I'm not entirely sure if this will be a breaking change or not. Using MathNode is still the public API, but it's definition changed. Are there current use cases of MathNode that will break?

For reference: this will resolve #2810

@mattvague
Copy link
Contributor Author

Do we need to describe how to implement a new Node class in TS in the documentation?

Ah, yes we do -- will do that. Where do you think the most appropriate place is? Should we maybe start a new page dedicated to Typescript?

I'm not entirely sure if this will be a breaking change or not. Using MathNode is still the public API, but it's definition changed. Are there current use cases of MathNode that will break?

Not totally sure yet either to be honest, but I think it would pay to be cautious. I'm going to pull these changes into my own project first and see what happens, I wonder if we could round up a few other projects using mathjs + typescript extensively and see if it breaks anything for them?

@josdejong
Copy link
Owner

Ah, yes we do -- will do that. Where do you think the most appropriate place is? Should we maybe start a new page dedicated to Typescript?

I think the it would be good to add a section to the following page: /docs/expressions/customization.md. Does that make sense?

Not totally sure yet either to be honest, but I think it would pay to be cautious. I'm going to pull these changes into my own project first and see what happens, I wonder if we could round up a few other projects using mathjs + typescript extensively and see if it breaks anything for them?

Sounds good. @ChristopherChudzicki would it be possible for you to try out this PR on your project to see if it breaks something?

interface ArrayNode<TItems extends MathNode[] = MathNode[]> extends MathNode {
interface ArrayNode<TItems extends BaseNode[] = BaseNode[]> extends BaseNode
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there may have been an error resolving a merge conflict or something... There's a { missing here which is resulting in some type errors.

@ChristopherChudzicki
Copy link
Contributor

ChristopherChudzicki commented Nov 23, 2022

Ack, sorry I missed the pings. I'm having a little trouble testing this out because I think there was an error resolving a merge conflict (see comment), and now the index.d.ts file is not valid.

@josdejong My expectation is that this

  • v11.2 --> v11.3 contained a breaking change, changing type MathNode from a union (AccessorNode | ArrayNode | ... ) to a looser interface.
  • This essentially restores the union, but makes it extensible
  • So this would be breaking relative to 11.3 (because it changes how extension is performed) but NOT breaking relative to 11.2

All in all, I think this should be classified as "Not breaking" because it only breaks how extension is performed. Upgrading from 11.2 to this version would have, I suspect, a smaller impact1 on types than upgrading from 11.2 to 11.3.

@mattvague Again, sorry for being slow. Had I been faster, maybe we'd have avoided the conflict all together. I should have much more time for the next 5 days or so. I'm happy to help resolve the issue, though I'm not sure the etiquette: would you want another PR into your branch? (Or happy to leave it to you.)

Footnotes

  1. Assumption: fewer people have begun extending MathNode than rely on the discriminated union.

@josdejong
Copy link
Owner

Thanks for your clear feedback @ChristopherChudzicki . It makes sense to me to not mark this as a breaking change, but as a bug fix of the regression introduced in v11.3. In some way, every bug fix is a breaking change, since it alters the original behavior 😜 .

@mattvague can you still write a paragraph in the documentation page /docs/expressions/customization.md?

@josdejong
Copy link
Owner

@mattvague do you see any chance finishing up this PR? If not I can see if I can write the docs.

@ChristopherChudzicki
Copy link
Contributor

ChristopherChudzicki commented Jan 2, 2023

TLDR: This only partially brings back the discriminated union, and I think the "only partially" makes the result confusing, and so maybe this isn't worth it.


With the goal of finishing up this PR (or starting a new one), I just took another look at this, which revealed some issues/features of the current approach that I think could be confusing.

For example, as a user consuming the library:

// Reminder: 
//  - BaseNode = generic interface
//  - MathNode = extensible collection that supports discriminated unions

const myFunctions = (node: MathNode) => {
    node.traverse(child => {
        // child has type MathNode, so discriminated unions work.
        // Cool!
    })


    if (node.type === 'ArrayNode') {
        // "items" has type BaseNode[]
        // Surprising/Confusing! Why isn't items MathNode[] ???
        const items = node.items
    }
}

As an end user, I would expect to always get a MathNode, and I would be confused why sometimes I get just a BaseNode (whose node.type property is just "string", so won't support discriminated unions.)

I would love to have the discriminated union back: it is very convenient and makes node types much more discoverable. But IMO, the combination of

  • unpredictability of "sometimes I get BaseNode, sometimes I get MathNode"
  • increased complexity from supporting an extensible discriminated union
    might make this not worth it.

Can the unpredictability be removed?: Maybe, but I can't figure out how. Everything I've tried leads to TS complaining about circularly defined types. (E.g., changing the default type parameter of ArrayNode here to MathNode leads to TS complaining.)

@josdejong
Copy link
Owner

Thanks for digging further @ChristopherChudzicki . So, would this PR be a (little) step in the right direction you think? Or better to start from scratch?

@mattvague
Copy link
Contributor Author

@mattvague do you see any chance finishing up this PR? If not I can see if I can write the docs.

Hi there @josdejong, very sorry for disappearing. Unfortunately I don't see being able to work on this in the next few weeks, but if someone else can take the charge I'm happy to provide feedback if wanted. I agree the inconsistency between BaseNode and MathNode is confusing and the reason we ended up doing that (if I recall) was exactly the "circularly defined types." issue @ChristopherChudzicki mentioned, I'm not sure what to do about that at the moment. Maybe it's best to check some other OSS libraries with a similar concept and see what they do?

@josdejong
Copy link
Owner

Thanks for getting back @mattvague. No problem.

Is there anyone else who can pick this up? Help would be very welcome.

@ChristopherChudzicki
Copy link
Contributor

I have been looking at this off and on. I really don't have an answer, but wanted to post the following minimal-ish reproduction of the issue: TS playground

We seem to have three goals (for this issue):

  1. MathNode type is extensible, users can add more node types
  2. MathNode type supports discriminated union
  3. individual node types support generic parameters whose value might be MathNode (e.g., the TItems parameter of ArrayNode).

Dropping any one of these three makes the problem significantly easier—Currently, we have 1 & 3 (and previously had 2 & 3).

All of this is to say... I'm a bit stuck on this. I'll keep an eye out for potential solutions, but probably won't be actively working on it. For now, I'm just going to start narrowing types by the predicates (isFunctionAssignmentNode, etc, as initially discussed in #2810) and upgrade to the current version of MathJS.

@josdejong
Copy link
Owner

Thanks for working out this minimal example that demonstrates the TS issues to make this work! This helps a lot. I had a short look but I do not yet see a magic solution either with my limited TS knowledge.

We opened #2810 again because we wanted to investigate whether we could make discriminated unions work too without sacrificing anything else, but it looks like that is not possible. I think we can live without discriminated unions and use typeguards to narrow down on types (see #2810 (comment)), so the current 2&3 is the second best?

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

Successfully merging this pull request may close these issues.

3 participants