Skip to content
This repository has been archived by the owner on Jul 16, 2024. It is now read-only.

align Interrupt apis and add optional interruption to async api #555

Merged

Conversation

jessekelly881
Copy link
Contributor

@jessekelly881 jessekelly881 commented Jul 21, 2023

Effect.async and Effect.asyncInterrupt can be merged into 1 function.

@changeset-bot
Copy link

changeset-bot bot commented Jul 21, 2023

🦋 Changeset detected

Latest commit: f247355

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@effect/io Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jessekelly881 jessekelly881 force-pushed the feat/merge-async-asyncInterrupt branch 3 times, most recently from 962b3ba to 5350d4d Compare July 21, 2023 19:56
@jessekelly881 jessekelly881 changed the title feat(Effect): merged async, asyncInterrupt feat(Effect): merge async, asyncInterrupt Jul 21, 2023
@IMax153
Copy link
Member

IMax153 commented Jul 21, 2023

@jessekelly881 - I'm not sure this is a good idea.

Returning void | Effect risks hard to find bugs, for example it completely allows a user to do:

Effect.async((resume) => {
  // no `return` statement
  Effect.sync(() => {})
})

cc @mikearnaldi for further input

@jessekelly881
Copy link
Contributor Author

jessekelly881 commented Jul 21, 2023

True. The most "correct" return type would probably be Option<Effect>. But.. that would be awkward to work with when not returning a value.

@mikearnaldi
Copy link
Member

I kind of like it tbh, dangling & unassigned effects could be flagged by an eslint plugin, this isn't the only case, for example logging in a generator has the same issue

@tim-smart
Copy link
Member

I quite like this too :)

Will take a look at moving this forward shortly.

@tim-smart tim-smart marked this pull request as ready for review July 26, 2023 22:03
@tim-smart tim-smart changed the title feat(Effect): merge async, asyncInterrupt update async and asyncInterrupt apis Jul 26, 2023
@tim-smart tim-smart changed the title update async and asyncInterrupt apis align Interrupt apis and add optional interruption to async apis Jul 26, 2023
@jessekelly881
Copy link
Contributor Author

Damn that was fast. Jaja. Looks good. :)

@tim-smart
Copy link
Member

@mikearnaldi Aligned the *Interrupt apis to use AbortSignal, and renamed asyncEither.

I wonder if they should be called asyncAbort, tryPromiseAbort instead.

@tim-smart tim-smart force-pushed the feat/merge-async-asyncInterrupt branch from d9f449b to f247355 Compare July 26, 2023 22:09
@tim-smart tim-smart changed the title align Interrupt apis and add optional interruption to async apis align Interrupt apis and add optional interruption to async api Jul 27, 2023
@mikearnaldi
Copy link
Member

@mikearnaldi Aligned the *Interrupt apis to use AbortSignal, and renamed asyncEither.

I wonder if they should be called asyncAbort, tryPromiseAbort instead.

I don't think so we should not have two names for interrupt they are semantically the same thing

@tim-smart tim-smart force-pushed the feat/merge-async-asyncInterrupt branch from c0c86c5 to f247355 Compare July 27, 2023 09:37
@tim-smart
Copy link
Member

I don't think so we should not have two names for interrupt they are semantically the same thing

Cool, will stick with interrupt.

@tim-smart tim-smart merged commit 29e4148 into Effect-TS:main Jul 27, 2023
1 check passed
@github-actions github-actions bot mentioned this pull request Jul 27, 2023
This was referenced Sep 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants