core(tsc): make CPUNode and NetworkNode a discriminated union #5548
Conversation
other options:
|
|
er, and last option, we don't do this and keep casting. Not a big deal if not :) |
I'm probably not getting it 100% but you will always need some kind of |
| const CPUNode = require('../../../lib/dependency-graph/cpu-node'); // eslint-disable-line no-unused-vars | ||
| const NetworkNode = require('../../../lib/dependency-graph/network-node'); // eslint-disable-line no-unused-vars | ||
|
|
||
| /** @typedef {Node.NodeType} NodeType */ |
There was a problem hiding this comment.
is this a necessary thing, or just prefer it over Node.NodeType everywhere?
There was a problem hiding this comment.
I guess I prefer it over Node.NodeType, but honestly it had become rote c/p by this file so I didn't really think of that possibility when both are needed in a file :)
| @@ -5,6 +5,12 @@ | |||
| */ | |||
| 'use strict'; | |||
|
|
|||
| /** | |||
| * A union of all types derived from Node, allowing type check disrimination | |||
| /** | ||
| * A union of all types derived from Node, allowing type check disrimination | ||
| * based on `node.type`. If a new node type is created, it should be added here. | ||
| * @typedef {import('./cpu-node.js') | import('./network-node.js')} NodeType |
There was a problem hiding this comment.
how would you feel about calling this one Node and the real base class BaseNode or something? it feels weird to say that all those methods accept a thing of type NodeType instead of saying they expect a Node
There was a problem hiding this comment.
it feels weird to say that all those methods accept a thing of type
NodeTypeinstead of saying they expect aNode
it does feel weird but I couldn't think of a decent alternative name for Node :) This suggestion sounds good, will do
yes, true, I meant non-functional as in no more functionality than what you'd already be doing to tell if something was a |
@patrickhulce this is what I mentioned a while back (in #5072). The Node type passed around becomes a union of
CPUNode | NetworkNode, which means things inside e.g. anif (node.type !== 'network')block thatnodebecomes aCPUNodewith no cast or anything.The downsides are
a) naming (but we can tweak) and
b) double casts of
thiswithin theNodeclass since the compiler doesn't know thatthisisn't an incompatible subclass ofNode(so first cast toNode, then cast toCPUNode | NetworkNode.