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.

Sign up for GitHub

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

Cc/discriminated unions #2

Merged
merged 5 commits into from
Nov 4, 2022
Merged

Cc/discriminated unions #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0fb3bf7
lint ts files in test dir
ChristopherChudzicki Oct 22, 2022
8928cb3
change operatornode generic defaults
ChristopherChudzicki Oct 22, 2022
ef72237
remove TOp type generic type
ChristopherChudzicki Oct 22, 2022
bdf864d
add a test for discrimination
ChristopherChudzicki Oct 22, 2022
899115d
revert to old operatornode typeargs
ChristopherChudzicki Nov 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .eslintrc.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ module.exports = {
}
},
{
files: ['types/*.ts'],
files: ['types/*.ts', 'test/**/*.ts'],
Copy link
Author

Choose a reason for hiding this comment

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

This is unrelated, but I noticed that the the TS tests weren't being linted since they moved directories.

Copy link

Choose a reason for hiding this comment

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

Nice find. We probably want to move that to a separate PR though?

Copy link

Choose a reason for hiding this comment

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

It's a tiny enough change that I for one don't mind throwing it in here but of course if Jos objects...

Copy link
Author

Choose a reason for hiding this comment

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

Mostly added it in this PR because the lack of linting annoyed me 😄 I went ahead and opened a separate PR for this change just to (possibly) get it in sooner. Should have done that last week 🤷

extends: [
'plugin:@typescript-eslint/recommended',
'plugin:prettier/recommended' // this should come last
Expand Down
52 changes: 52 additions & 0 deletions test/typescript-tests/nodeExtensions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { expectTypeOf } from 'expect-type'
import {
MathNode,
BaseNode,
MathNodeCommon,
ConstantNode,
Node,
FunctionAssignmentNode,
} from 'mathjs'

interface MyNode extends BaseNode {
type: 'MyNode'
a: MathNode
}
declare module 'mathjs' {
interface MathNodeTypes {
MyNode: MyNode
}
}
Comment on lines +11 to +19
Copy link
Author

Choose a reason for hiding this comment

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

I didn't see this declaration in your branch...We would not expect some of the tests below to pass without it, right?

TS told me that ambient declarations need to be at the top level of nesting within a file, and I didn't want this to affect the other tests, so I moved this test to a separate file.

Copy link

Choose a reason for hiding this comment

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

I didn't see this declaration in your branch...We would not expect some of the tests below to pass without it, right?

Yep, think you're right.

TS told me that ambient declarations need to be at the top level of nesting within a file, and I didn't want this to affect the other tests, so I moved this test to a separate file.

Yeah separate file seems fair. We are aiming to start splitting this up anyways.


/*
MathNode examples
*/
{
class CustomNode extends Node implements MyNode {
a: MathNode
type: 'MyNode'
constructor(a: MathNode) {
super()
this.a = a
}
}

// Built-in subclass of Node
const instance1 = new ConstantNode(2)

// Custom subclass of node
const instance2 = new CustomNode(new ConstantNode(2))

expectTypeOf(instance1).toMatchTypeOf<MathNode>()
expectTypeOf(instance1).toMatchTypeOf<MathNodeCommon>()
expectTypeOf(instance1).toMatchTypeOf<ConstantNode>()

expectTypeOf(instance2).toMatchTypeOf<MathNode>()
expectTypeOf(instance2).toMatchTypeOf<MathNodeCommon>()
expectTypeOf(instance2).toMatchTypeOf<CustomNode>()

let instance3: MathNode
if (instance3.type === 'FunctionAssignmentNode') {
expectTypeOf(instance3).toMatchTypeOf<FunctionAssignmentNode>()
}
}
42 changes: 4 additions & 38 deletions test/typescript-tests/testTypes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,9 @@ import {
SimplifyRule,
SLUDecomposition,
SymbolNode,
MathNodeCommon,
Unit,
Node,
isSymbolNode,
BaseNode,
} from 'mathjs'
import * as assert from 'assert'
import { expectTypeOf } from 'expect-type'
Expand Down Expand Up @@ -1098,7 +1097,7 @@ Transform examples

expectTypeOf(
math.parse('sqrt(3^2 + 4^2)').transform(myTransform1)
).toMatchTypeOf<OperatorNode<'+', 'add', MathNode[]>>()
).toMatchTypeOf<OperatorNode<'+', 'add', BaseNode[]>>()

assert.deepStrictEqual(
math.parse('sqrt(3^2 + 4^2)').transform(myTransform1).toString(),
Expand All @@ -1110,7 +1109,7 @@ Transform examples
.parse('sqrt(3^2 + 4^2)')
.transform(myTransform1)
.transform(myTransform2)
).toMatchTypeOf<OperatorNode<'-', 'subtract', MathNode[]>>()
).toMatchTypeOf<OperatorNode<'-', 'subtract', BaseNode[]>>()

assert.deepStrictEqual(
math
Expand Down Expand Up @@ -2226,7 +2225,7 @@ Factory Test
}
if (math.isOperatorNode(x)) {
expectTypeOf(x).toMatchTypeOf<
OperatorNode<OperatorNodeOp, OperatorNodeFn, MathNode[]>
OperatorNode<OperatorNodeOp, OperatorNodeFn, BaseNode[]>
>()
}
if (math.isParenthesisNode(x)) {
Expand Down Expand Up @@ -2308,36 +2307,3 @@ Random examples
MathJsChain<number[]>
>()
}

/*
MathNode examples
*/
{
class CustomNode extends Node {
a: MathNode
constructor(a: MathNode) {
super()
this.a = a
}
}

// Basic node
const instance1 = new Node()

// Built-in subclass of Node
const instance2 = new ConstantNode(2)

// Custom subclass of node
const instance3 = new CustomNode(new ConstantNode(2))

expectTypeOf(instance1).toMatchTypeOf<MathNode>()
expectTypeOf(instance1).toMatchTypeOf<MathNodeCommon>()
Comment on lines -2333 to -2334
Copy link
Author

Choose a reason for hiding this comment

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

If we're trying for a discriminated union on MathNode, then I don't think this test should pass: instance1 is a BaseNode, and I don't think a BaseNode should be assignable to a MathNode: BaseNodes. The type property on BaseNode is type: string, whereas on all other nodes it's a constant.

If BaseNode is assignmable to MathNode, then

let node: MathNode
if (node.type === 'AssignmentNode) {
    // now node has type BaseNode | AssignmentNode
}


expectTypeOf(instance2).toMatchTypeOf<MathNode>()
expectTypeOf(instance2).toMatchTypeOf<MathNodeCommon>()
expectTypeOf(instance2).toMatchTypeOf<ConstantNode>()

expectTypeOf(instance3).toMatchTypeOf<MathNode>()
expectTypeOf(instance3).toMatchTypeOf<MathNodeCommon>()
expectTypeOf(instance3).toMatchTypeOf<CustomNode>()
}
2 changes: 1 addition & 1 deletion types/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ declare namespace math {
FunctionNode: FunctionNode
IndexNode: IndexNode
ObjectNode: ObjectNode
OperatorNode: OperatorNode
OperatorNode: OperatorNode<OperatorNodeOp, OperatorNodeFn>
ParenthesisNode: ParenthesisNode
RangeNode: RangeNode
RelationalNode: RelationalNode
Expand Down