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

migrate events #695

Merged
merged 9 commits into from
May 26, 2023
Merged

migrate events #695

merged 9 commits into from
May 26, 2023

Conversation

dagda1
Copy link
Contributor

@dagda1 dagda1 commented Apr 18, 2023

Motivation

Migrate on and once to v3 and add better typing around the event type.

Approach

Extract all the onXXX events from the event target and allow a fallback to string if none is found.

Copy link
Member

@cowboyd cowboyd left a comment

Choose a reason for hiding this comment

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

What do you think about a curried API that can be used with a pipe helper a-la https://gist.github.com/cowboyd/384b456e7093462ac0b1c7b33d6213d7

In other words on('message')(websocket) instead of on(websocket, message)

lib/deps.ts Outdated
@@ -1,2 +1,3 @@
export { assert } from "https://deno.land/std@0.158.0/testing/asserts.ts";
export * from "https://deno.land/x/continuation@0.1.5/mod.ts";
export { expectType } from "https://esm.sh/ts-expect@1.3.0";
Copy link
Member

Choose a reason for hiding this comment

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

This should probably not be part of the main deps, but maybe put it in test/suite.ts? That way, when we bundle it for npm for example, it won't be included.

Copy link
Contributor Author

@dagda1 dagda1 Apr 18, 2023

Choose a reason for hiding this comment

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

good point, I will change it.

I actually just thought, that I did not bring the tests from the v2 in.

I'll need to add those.

Is the external API the same and only the internals different?

@dagda1
Copy link
Contributor Author

dagda1 commented Apr 18, 2023

In other words on('message')(websocket) instead of on(websocket, message)

I'm a big fan of pipe so I like it.

Do you think it makes sense to support both?

on('message')(websocket) and on('message')(websocket). I'm thinking that most people know the latter.

@cowboyd
Copy link
Member

cowboyd commented Apr 18, 2023

Do you think it makes sense to support both?

We could try and have overloads? How difficult does that make it for you to work out the types?

@dagda1
Copy link
Contributor Author

dagda1 commented Apr 19, 2023

https://gist.github.com/cowboyd/384b456e7093462ac0b1c7b33d6213d7

Any pipe I have used is a generic function that does all the type magic for you, e.g fp-ts so you would not need to overload anything.

You just need an effection aware pipe that does all the nice type crazyness.

Gist
Piping, buffering, filtering, mapping, and general hypothetical combinations API. - buffer.ts

@cowboyd
Copy link
Member

cowboyd commented Apr 19, 2023

pipe and flow from fp-ts is pretty much exactly what we want.

I'm imagining that you would use pipe even for very simple stream iterations:

import { on, pipe } from "effection";
    
let websocket = new WebSocket('wss://localhost:9000');
  
yield* pipe(websocket, on("message"), forEach(function*(message) {
  console.log('received message from websocket', message)
}));

Something about that feels kinda right. Everything is just a combinator, even forEach. So simple iterations and complex iterations all have the same structure:

import { buffer, on, pipe, filter } from "effection";
  
yield* pipe(
  new WebSocket('wss://localhost:9000'),
  on('message'),
  buffer({ size: 100 }),
  map(message => JSON.parse(message.data)),
  filter(data => data.type === "result"),
  forEach(function*(result) {
    console.log('received result from websocket', result)  
  })
);

Perhaps we should get #693 sorted before we get this put in.

@dagda1
Copy link
Contributor Author

dagda1 commented Apr 19, 2023

Perhaps we should get #693 sorted before we get this put in.

I think Pipe and flow should be a separate PR.

@dagda1
Copy link
Contributor Author

dagda1 commented Apr 19, 2023

I think it would be seriously cool to have them here

lib/events.ts Outdated
Comment on lines 46 to 64
export function once<
T extends EventTarget,
K extends EventList<T> | (string & {}),
>(target: T, name: K): Operation<EventTypeFromEventTarget<T, K>> {
return action(function* (resolve) {
target.addEventListener(
name,
resolve as EventListenerOrEventListenerObject,
);
try {
yield* suspend();
} finally {
target.removeEventListener(
name,
resolve as EventListenerOrEventListenerObject,
);
}
});
}
Copy link
Member

@cowboyd cowboyd May 5, 2023

Choose a reason for hiding this comment

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

Can we implement this using first() on the stream?

Copy link
Member

@cowboyd cowboyd left a comment

Choose a reason for hiding this comment

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

A couple of items:

  1. These should be compatible with pipe(), so that you can say pipe(websocket, on("message")). That means individually, it would be on("message")(websocket).
  2. I don't think we should support EventEmitter anymore since now almost every event emitter is also a NodeEventTarget. It caused troubles when you had event callbacks that passed multiple arguments and led to us having to make a onEmit and onceEmit variations. If folks want to use the event emitter api we can provide a separate subscription mechanism.
  3. I think we can express once() in terms of first() composed with on. i.e. the "first" item in the event stream:
function once(target, eventName) {
  return first(on(eventName)(target));
}

Should once() be a combinator? I'm not sure.

@dagda1
Copy link
Contributor Author

dagda1 commented May 8, 2023

  1. These should be compatible with pipe(), so that you can say pipe(websocket, on("message")). That means individually, it would be on("message")(websocket).

This is where it gets interesting, in order for any function to work with pipe, it needs to be unary and it needs to be data last

So we would need to rewrite on and once like this:

function on2<K extends EventList<unknown> | (string & {})>(name: K) {
   return function<T extends EventTarget>(target: T): Stream<EventTypeFromEventTarget<T, K>, never> { {

We lose all the typing because we rely on the target to get the list of onXXXX event names for the nice autocomplete.

@dagda1
Copy link
Contributor Author

dagda1 commented May 8, 2023

function once(target, eventName) {
  return first(on(eventName)(target));
}

nice!

@dagda1
Copy link
Contributor Author

dagda1 commented May 8, 2023

Should once() be a combinator? I'm not sure.

Can I get a definition of combinator in this context? Do you mean once would be a higher order function like map and filter?

If we want anything to work with pipe then I think anything needs to be a combinator or curried.

@cowboyd
Copy link
Member

cowboyd commented May 8, 2023

@dagda1 combinator is as you say, a unary function that allows this to be used with pipe. I feel like on() should be a combinator, but should once()?

@cowboyd
Copy link
Member

cowboyd commented May 8, 2023

We lose all the typing because we rely on the target to get the list of onXXXX event names for the nice autocomplete.

😭 😭 😭

Do we not get typing because the function returned is parameterized?

return function<T extends EventTarget>(target: T): Stream<EventTypeFromEventTarget<T, K>, never> {

@cowboyd
Copy link
Member

cowboyd commented May 8, 2023

In other words, it should be able to infer if we say:

pipe(websocket, on("mess // => runtime knows that "message" is the only valid event name

right?

@dagda1
Copy link
Contributor Author

dagda1 commented May 8, 2023

In other words, it should be able to infer if we say:

right?

I'm not sure typescript can infer data last functions. It will work perfectly fine with pipe and everything will get inferred but if you were to call the function without pipe then the inference will not work

I have a passing and failing test for both

ccc

I'll do a bit more digging. I hope I'm wrong about this.

@dagda1
Copy link
Contributor Author

dagda1 commented May 9, 2023

@cowboyd I've overloaded on and once so at least this type checks

expectType<Operation<CloseEvent>>(pipe(socket, once("close")));

expectType<Operation<MouseEvent>>(once("click", domElement));

Everything is beautiful with pipe but maybe less beautiful without.

@dagda1
Copy link
Contributor Author

dagda1 commented May 22, 2023

@cowboyd I've removed the onFP and onceFP and all the craziness associated with them

Copy link
Member

@cowboyd cowboyd left a comment

Choose a reason for hiding this comment

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

🥳 I love the strong typing so that you get completion on the event names! and combined with the yield* syntax, where you get strongly type event values, it really does make a difference.

let event = yield* once(websocket, 'close');

@dagda1 dagda1 merged commit 67dab12 into v3 May 26, 2023
1 check passed
@dagda1 dagda1 deleted the pc/events branch May 26, 2023 21:31
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.

None yet

2 participants