Closed cassiozen closed 3 years ago
I'll look into it, though you're in the non-stale rewrite
right?
The weird part is that nextEvents type looks correct "START"[] | "PAUSE"[] | ("START" | "RESET")[] | undefined
That indeed is weird.
Yes, that's on rewrite
.
Welp, I'm afraid we can't do anything about this. Here's an explanation why it happens. TypeScript is doing it's job as A[] | B[]
is not same as (A | B)[]
declare let x: "a"[] | "b"[]
x.includes("a")
// Error: Argument of type 'string' is not assignable to parameter of type 'never'.(2345)
/*
("a"[] | "b"[])["includes"]
= "a"[]["includes"] | "b"[]["includes"]
= ((search: "a") => boolean) | ((search: "b") => boolean)
= (search: "a" & "b") => boolean | boolean
= (search: never) => boolean
*/
const includes = <T extends any[]>(xs: T, x: T[number]) => xs.includes(x)
includes(x, "a")
// compiles
Some ways to go about this -
Degrade State
types to no longer be typestated (meaning no union, squash it, which results in loss of information) EDIT - we can keep the union just let nextEvents
be squashed which will result in less loss of information
Tell users to use this better includes
?
Patch include in the State
type like this...
declare let stateOriginal:
| { value: "a"
, nextEvents: "X"[]
}
| { value: "b"
, nextEvents: "Y"[]
}
declare let stateIncludeFriendly:
| { value: "a"
, nextEvents: "X"[] & { includes: ("X" | "Y")["includes"] }
}
| { value: "b"
, nextEvents: "Y"[] & { includes: ("X" | "Y")["includes"] }
}
// stateOriginal
// pro: errors if we're checking something that will never happen
if (stateOriginal.value === "a") {
stateOriginal.nextEvents.includes("Y")
// Error: Argument of type '"Y"' is not assignable to parameter of type '"X"'.(2345)
}
// con: this doesn't work
stateOriginal.nextEvents.includes("X")
// Error: Argument of type 'string' is not assignable to parameter of type 'never'.(2345)
// stateIncludeFriendly
// con: does not error if we're checking something that will never happen
if (stateIncludeFriendly.value === "a") {
stateIncludeFriendly.nextEvents.includes("Y")
}
// pro: this works
stateIncludeFriendly.nextEvents.includes("X")
// If JavaScript was more functional-esque and had a static `Array.include`,
// wouldn't have the con in former and still keep the pro
Provide something like normalise
...
declare let x: "a"[] | "b"[]
const normalise = <T extends any[]>(xs: T) => xs as T[number][]
normalise(x).includes("a")
Patch array to have a normalised version in property $
. EDIT - or instead of hacks like his provide both nextEvents
and nextEventsN
.
declare let x: "a"[] | "b"[]
const withNormalised: <T extends any[]>(xs: T) => {
xs.$ = xs;
return xs as T & { $: T[number][] }
}
let nextEvents = withNormalised(x) // will be done internally
nextEvents.$.includes("a")
I actually like 5 or 4, because the problem is not with includes
per say all array prototypes that take element as an parameter will face this issue for example push
, indexOf
, etc. So users need a way to gain the normalised (not sure if that's a good name) version.
Wow, I understand now - thanks for the detailed summary.
When possible, I'll always prefer the option that introduces less API. I honestly can't think of a use case where a typestated nextEvents
would be useful, so I think squashing it makes more sense.
So I guess for me it's option 1, but only degrading the type for nextEvents
.
@devanshj I can probably tackle this one myself, unless you have a strong opinion that degrading the nextEvents type is not the way to go.
unless you have a strong opinion that degrading the nextEvents type is not the way to go.
I do think that. Because we really don't know what users would do with nextEvents
it should be open to possibilities. Let's say they are doing stuff like state.value === "x" && state.nextEvents.map(e => <Button onClick={() => f(e)}>{e}</Button>)
, here the implementation of f
should be able to leverage the nextEvents
of state x
only.
And let's say we squash it, nextEvents
gets widened it means the user would narrow it via an assertion in f
. And assertions are always prone to error because they mean you're asserting you know more than the compiler -- which can cause runtime bugs. So let's say the user asserts e
in f
to be XEvent | YEvent
but then later edits the machine in a way that e
could also be ZEvent
but they forgot to update the assertion -- and there we have a runtime error.
And anyway, I really don't think it's fair to trade precise types for not having an extra property imo. I'd suggest we go with nextEvents
and nextEventsN
, or if you want the default to be normalised then nextEvents
and nextEventsT
(T for typed).
But again, it's your library you get to take the final call :P
Edit - also in case it's not obvious nextEvents
and nextEventsN
will have the same runtime value, just different types.
If we are indeed adding to the API (and your example convinced me) I'd prefer to have more distinct naming.
Food for thought: I already want to introduce a matches
method so the user can have a single, consistent way to compare the current state to a string (#44).
Maybe we could keep the nextEvents
array correctly typed and introduce a handles
method (which is pretty much your better includes
.
I'd prefer to have more distinct naming.
Actually I'd suggest the exact opposite. You see more distinct the name is, more it looks as if does it something else or is something else; but in our case it's literally the same thing, just a difference of its type.
And adding a single character change has prior art -
fp-ts
prefixes a bunch of functions with W
(for widen) to denote less strict version of the same function without W
- ref'
to indicate it's a more strict version or just simply even a variant. - reff(x)
as f'(x)
- refSo I'd strongly suggest to not keep a distinct name, it'd set up wrong expectations. Let the users enter the domain of fp & types and experience naming they haven't seen before :P
Food for thought: I already want to introduce a matches method so the user can have a single, consistent way to compare the current state to a string (#44).
Maybe we could keep the nextEvents array correctly typed and introduce a handles method (which is pretty much your better includes.
I think that would be solving the wrong problem, as I said it's not just about include
-- something more fundamental is missing here, include
issue is just a manifestation of that.
Ok, makes sense. Let's go with the normalized version as default and nextEventsT for the typed one.
Done!
While updating the timer example, I noticed this bug:
The weird part is that
nextEvents
type looks correct"START"[] | "PAUSE"[] | ("START" | "RESET")[] | undefined
@devanshj