#Soundness issue

172 messages · Page 1 of 1 (latest)

plush crystal
# 
function test(listener: (...args: any[]) => void) {
    listener(123, 'hello');
}

// Prints "number"
test((s: string) => { console.log(typeof s); });
cyan plume
#

right, that's what any does. it turns off ts.

plush crystal
#

So I should perhaps put unknown instead here?

cyan plume
#

don't use any and you won't get that problem

lusty sunBOT
#
that_guy977#0
Playground Link

Preview:```ts
function test(
listener: (...args: [number, string]) => void
) {
listener(123, "hello")
}

function test2(
listener: (...args: unknown[]) => void
) {
listener(123, "hello")
}

test((s: string) => {
console.log(typeof s)
})
test2((s: string) => {
console.log(typeof s)
})```

You can choose specific lines to embed by selecting them before copying the link.
cyan plume
#

not "should be unknown" though. what you should use is what it should really be
might be unknown, might be something else; either way, probably not any.

plush crystal
#

yes, sorry the context of this is some complex types I'm adding to @types/node.

cyan plume
#

if you have a more specific case regarding that, could you provide more details

plush crystal
#

In https://github.com/DefinitelyTyped/DefinitelyTyped/pull/68313/files#diff-43c88aad0d46b05d2e147b5e4ff2166600faf5db2e6d4795234b19b3e76a3587, I originally had event5: any[].

But then I couldn't provoke an error further down, if I plugged in a less compatible listener to this event.

GitHub
[node] Add generics to 'EventEmitter' by malthe · Pull Request #683...

Please fill in this template.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Add or edit tests to re...

#

And I thought, well why not? I didn't know any disabled ts in this way; and actually, I still don't understand exactly why it must behave like this.

cyan plume
#

any is not within the type system, it's a special type that acts as an escape hatch for when you can't statically specify external data (although, it's recommended to use unknown in this case instead)

#

the reason any disables ts is because it's specifcally designated as the type that turns off ts

plush crystal
#

I wonder if there's a real need for that, or if it's to help frustrated programmers get the damn thing to compile.

#

vs an any that's simply the "top type".

verbal moat
#

any exists so to make it easier to migrate from JS to TS.

#

any is not really the top type, any simply turns off the type system, so it's kind of like "simultaneously top and bottom type"

#

The true top type in TS is unknown, and the true bottom type is never.

#

Do be careful though, (...args: unknown[]) => void is a really narrow type

#

There are very few functions that can be assigned to it, so make sure that's what you really want.

#

I have a feeling that you want a really wide type rather than that.

plush crystal
#

yeah what would the consequence of that be – let's say, in the context of an EventEmitter listener.

#

Broader context: I'm making it possible to say EventEmitter<EventMap> to avoid this whole mess.

#

But in the meantime, lots of code depends on the "broader" EventEmitter type definition.

#

(Which has any as rest args type.)

#

I guess it's a good compromise for users until they're able to adopt this new EventEmitter with generics.

cyan plume
#

since these are parameter types, they're contravariant. the type-safe equivalent to any[] here is never.

verbal moat
#

I think you are misunderstanding

#

if you want a broad function type, you don't have to resort to any.

cyan plume
#

btw, looking at your pr, as noted in the issue template, Function is probably not what you want either.

plush crystal
#

I didn't want to change any existing tests, I wanted to keep the old behavior 1:1.

#

So that only if users were to provide an event map type would they get a new behavior.

cyan plume
#

you can put any[] as the defaults when no type arguments are supplied

plush crystal
#

Yes that is basically what is happening now.

cyan plume
#

or i guess Record<string, any[]> actually

cyan plume
plush crystal
#

There is a lot of any but all that is necessary to keep the current behavior.

verbal moat
#

(...args: never) => unknown is the upper bound for function type, so it's the broadest function type that can accept any function, yet it remains safe that you cannot call it.

plush crystal
#

Which is necessary, otherwise lots and lots of types need to be changed.

cyan plume
plush crystal
#

I have tried different approaches, what I have now is what makes the existing tests (and 50-something packages that depend on EventEmitter) run unchanged.

#

Would be nice if there was something even simpler.

verbal moat
#

But yeah just to make my point clear, look at the following 3 examples

lusty sunBOT
# 
function test(listener: (...args: any[]) => void) {
    listener(123, 'hello')
}

test(() => {})
test((foo: string) => {})
test((foo: string, bar: number) => {})
# 
function test(listener: (...args: never) => void) {
    listener(123, 'hello')
//           ^^^^^^^^^^^^
// Argument of type '[number, string]' is not assignable to parameter of type 'never'.
}

test(() => {})
test((foo: string) => {})
test((foo: string, bar: number) => {})
# 
function test(listener: (...args: unknown[]) => void) {
    listener(123, 'hello')
}

test(() => {})
test((foo: string) => {})
//   ^^^^^^^^^^^^^^^^^^^
// Argument of type '(foo: string) => void' is not assignable to parameter of type '(...args: unknown[]) => void'.
//   Types of parameters 'foo' and 'args' are incompatible.
//     Type 'unknown' is not assignable to type 'string'.
test((foo: string, bar: number) => {})
//   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
// Argument of type '(foo: string, bar: number) => void' is not assignable to parameter of type '(...args: unknown[]) => void'.
//   Types of parameters 'foo' and 'args' are incompatible.
//     Type 'unknown' is not assignable to type 'string'.
verbal moat
#

(...args: never) => is what you want, if you want to accept any function while remain safe.

#

(...args: unknown[]) => is actually wrong, because it's a really narrow function type that accepts practically nothing.

#

And obviously, (...args: any[]) => is unsafe.

#

But this is strictly from caller's PoV (code that calls test), it might be a different story from implementor's PoV (test's implementation that wants to call listener)

#

Using (...args: never) => is completely safe, but that safeness also means from implementer's PoV, test can never ever call listener. After all, if test allows any function to be passed in, then there's no way for test to know how it can safely call listener so there's no way to call at all. That's might be a tradeoff you care about.

cyan plume
#

btw, the discussion by eritbh does mention that other projects have this pattern.
did you check any of those out?
djs for example has an implementation of a system with strict events, and it really only uses 3 extra things

#

T extends EventMap<T> seems unnecessary, it could just be T extends EventMap where type EventMap = Record<PropertyKey, readonly unknown[]>, and type DefaultEventMap = Record<PropertyKey, any[]> like mentioned in the discussion

#

from skimming, looks like a lot of the utility types you've added aren't really necessary either?

plush crystal
#

That's how I started, using the suggested code. Then I changed to using a marker type for the default type because I couldn't make it work with something like Record<PropertyKey, readonly unknown[]> as the default.

cyan plume
#

K extends keyof T would work in place of what Key<K, T> is doing

plush crystal
#

That doesn't work either, the type system explodes if you use that approach.

#

Believe me, I've tried.

#

The Key<K, T> is what eventually made the thing actually work.

#

If you have a testing setup ready, it's pretty easy to experiment on, just run pnpm test node and you can quickly hit some of the many problems.

cyan plume
#

i thought ts was my enemy but it was git

#

give me a bit...

#

real quick, why are there 3 separate definitions for EventEmitter?

#

also, are _Node/_Dom EventTarget in scope?

#

also not sure what E is doing exactly; i know its intent, but not how it's meant to be used, could you expand on that?

#

if it's supposed to be the acceptable types for emit, should @@captureRejectionSymbol also use E instead of T?

#

also, what's with the string | symbol vs string discrepancy between the other methods and @@captureRejectionSymbol? guess that one's just typed wrong, the docs say string | symbol

cyan plume
plush crystal
#

I'm not totally sure why it's organized how it is to be honest with 3 separate definitions.

#

The captureRejectionSymbol I am not totally sure about.

#

So E is just anything that's emitted through emit which I suppose is everything. But as far as I understand, remembering now, captureRejectionSymbol that's for the listeners.

#

So if a listener fails, then you can capture that rejection?

#

In that case it should be T and not E right?

#

To be honest, it does seem weird that you could emit A and listen to something else.

#

But there's a package that does have that design and for a "sanity check" I wanted to see if it was possible to add types to some of the many packages that depend on EventEmitter (but currently provide their own APIs in a bunch of different ways).

#

It might be useful to drop this E stuff for now because it can always be added later if there's a real need.

cyan plume
plush crystal
#

I also thought perhaps DefaultEventMap is not the best name because the default is really anything goes.

cyan plume
#

eh, we can leave specifics for later.

#

anyways, as you might be able to tell, i'm really new to dev'ing for DT. and also pnpm.
how long would testing take, or could i specify only a few tests to run?

plush crystal
#

testing just node is fast, 2-3 secs.

#

$ pnpm test node

#

(After you install using pnpm install.)

#

First install pnpm itself using something like Homebrew.

#

That's what worked for me at least.

#

I have updated the branch now, dropping E altogether.

cyan plume
#

i tried with npx pnpm, seems to be going well.

#

testing is fast now. i don't know what changed tbh. it was hanging a few tries ago

plush crystal
#

Of course testing all of the implicated packages takes forever.

#

But one can iterate pretty well on just node.

cyan plume
#

also, found a case where type vs interface matters, damn.

#

no implicit signature moment...

plush crystal
#

yeah there are important differences there.

cyan plume
#

oh damn, just realized.

#

if a library uses an interface for their EventEmitter<T> and exposes it, you could just add events via that...

#

that's.. pretty cool, ngl. even if unintentional

plush crystal
#

How do you mean, not sure I completely follow.

cyan plume
#

Re: the discussion, on the topic of libraries allowing custom events.
you could safely define static events, by augmenting the interface that holds the EventMap, for example in djs:

import type { ClientEvents } from "discord.js";

interface ClientEvents {
  customEvent: [unknown]
}
plush crystal
#

oh I see right.

cyan plume
#

i love ts-server

image.png
#

so i've narrowed it down to 3 errors, one of which i'm moderately certain is a mistake in the original typings and an incorrect test

#

oh wait it's the same other 2 errors that i just solved

#

i modified some tests in events_generic, the ones that were modeled after your old EventListener<T, E> with the Listener stuff, and it's working fine now

plush crystal
#

did you pull the latest changes where I dropped out E entirely?

#

But I thought best to postpone that part

cyan plume
cyan plume
plush crystal
#

nice

#

I just thought that for a merge to be realistic, it's nice that tests just keep running.

#

but what you can do also is to run some of the many tests in packages via pnpm run test-all.

#

After 10 minutes, you should have a good idea if there are problems.

cyan plume
cyan plume
#

my computer is.. not the strongest

plush crystal
#

so individual packages, you can test quickly.

#

for example pnpm test twitter – it uses EventEmitter too.

cyan plume
#

i mean in size. i was fighting git to fetch only types/node earlier lol

plush crystal
#

ooh right but you could do a shallow fetch

#

that's much lighter

cyan plume
#

but aight, i'll add the ones modified in your original pr

plush crystal
#

git clone --filter=blob:none

#

^^^ should help but untested, and yeah your approach can work too.

cyan plume
#

i think i messed up git 🙃

image.png
plush crystal
#

you must rebase, that's easy.

#

what does git remote -v tell you

cyan plume
#

mmmmm how bout i ignore it... for now

#

i have no idea what to make of this tbh, i'm a git amateur

image.png
plush crystal
#

Try git pull rebase -i origin generic-events.

cyan plume
#

ehhh idk about that, i remember only fetching 1 commit by... something along the chain

plush crystal
#

git is tricky ...

cyan plume
#

i have the files so... i guess imma just put it off lol

plush crystal
#

nice

cyan plume
#

well, guess not

image.png
#

lmfao

#

The Art of the Bodge™️

plush crystal
#

strange works on mine

#

but I had problems with npx.

#

I had to get pnpm to exist globally instead. But I'm on a Mac as well so.

#

Ur on PC?

cyan plume
#

no, mac

#

mojave

cyan plume
#

anyways, i just realized i forgot to pnpm install and that's why i'm getting a ton of errors lmao

cyan plume
plush crystal
#

let's see if we can fix it without causing too much trouble.

cyan plume
#

it will be backwards incompatible, unfortunately

#

it's widening @@captureRejectionSymbol from string to string | symbol

#

no clue why it's optional though, i feel like it shouldn't be that

#

it's... probably also wrong?

#

idk

plush crystal
#

an EventEmitter supports event names that are string | symbol.

#

So I have captureRejectionSymbol also set up for that.

cyan plume
#

yeah, but it used to be just string, and the test in question expects that

#

it should be string | symbol, that's what node documents

#

the test in question is at .../types/node/test/events.ts:121

plush crystal
#

that test runs fine for me

#

but I can see how it should fail!

cyan plume
plush crystal
#

I think string | symbol is somehow assignable to string.

cyan plume
#

maybe we haven't converged on the same definitions

cyan plume
plush crystal
#

You can see from my PR that I haven't touched a single line in the tests.

#

And I do have string | symbol as the eventName type.

cyan plume
#

my current definitions break for opossum's Status

#

rip

#

it's getting late here so i'll probably just have to dump what i have into prs for you

#

note that the static methods of EventEmitter aren't all genericized yet and _NodeEventTarget and _DOMEventTarget haven't been touched but maybe should be

#

also changing DefaultEventMap to Record<any, any[]> solves the extension problem but breaks other use

#

also haven't touched the ones in v16 and v18

cyan plume
plush crystal
#

I don't really know what those v16 and v18 are about.