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

fix: tap and andThen fallthrough function #2264

Merged
merged 7 commits into from
Mar 8, 2024
Merged

Conversation

patroza
Copy link
Member

@patroza patroza commented Mar 8, 2024

fixes for example:

declare const foo: (x: number) => Effect.Effect<void>;
Effect.succeed(true).pipe(Effect.tap(foo)); // no error
Effect.succeed(true).pipe(Effect.tap(x => foo(x))); // error

Copy link

changeset-bot bot commented Mar 8, 2024

🦋 Changeset detected

Latest commit: 7789d37

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

This PR includes changesets to release 15 packages
Name Type
effect Patch
@effect/cli Patch
@effect/experimental Patch
@effect/opentelemetry Patch
@effect/platform-browser Patch
@effect/platform-bun Patch
@effect/platform-node-shared Patch
@effect/platform-node Patch
@effect/platform Patch
@effect/printer-ansi Patch
@effect/printer Patch
@effect/rpc-http Patch
@effect/rpc Patch
@effect/schema Patch
@effect/typeclass Patch

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

@patroza patroza changed the title B/fix tap and then fix: tap and andThen fallthrough function Mar 8, 2024
@patroza patroza marked this pull request as ready for review March 8, 2024 15:26
@mikearnaldi
Copy link
Member

Mmm what about Effects that are functions? Like the proxy ones from the new tag

@patroza
Copy link
Member Author

patroza commented Mar 8, 2024

Mmm what about Effects that are functions? Like the proxy ones from the new tag

What about them? It they match the supported function signature for andThen/tap, then they’re golden. Otherwise they are (I believe) rightfully rejected.

@mikearnaldi
Copy link
Member

I think if an arg is both a function and an effect we have to force () => because the impl will take a choice, either there is an isFunction or isEffect. Quite a conondrum...

@patroza
Copy link
Member Author

patroza commented Mar 8, 2024

I think if an arg is both a function and an effect we have to force () => because the impl will take a choice, either there is an isFunction or isEffect. Quite a conondrum...

I see, seems like a related topic, how is it affected by this change?

@mikearnaldi
Copy link
Member

mikearnaldi commented Mar 8, 2024 via email

@patroza
Copy link
Member Author

patroza commented Mar 8, 2024

It's not affected but related, with the intro of effect.tag we can't basically distinguish functions from effects, it is an issue for tacit usage

alright, would at least like to get the bug fix merged so that users stop getting bit by it.
In the meantime we can look into it. (Though I guess to distribute it we would need to revert Effect.tag then)
From my gut feeling though; isn’t the proxy hiding the internals on access? On phone. Would need some test cases I guess

@tim-smart
Copy link
Member

I think there are a few other places where we assume an Effect isn't a function, will need to hunt them down.

@tim-smart tim-smart merged commit e03811e into main Mar 8, 2024
12 checks passed
@tim-smart tim-smart deleted the b/fix-tap-and-then branch March 8, 2024 19:34
@github-actions github-actions bot mentioned this pull request Mar 8, 2024
@patroza
Copy link
Member Author

patroza commented Mar 8, 2024

It's not affected but related, with the intro of effect.tag we can't basically distinguish functions from effects, it is an issue for tacit usage

I see now yes, that's certainly 'fun' :D

@patroza
Copy link
Member Author

patroza commented Mar 8, 2024

It's not affected but related, with the intro of effect.tag we can't basically distinguish functions from effects, it is an issue for tacit usage

How about https://github.com/Effect-TS/effect/pull/2266/files#diff-1eaca66f74be66210f25595750619630e4d14443b5ddaa1f2f8a265256432778R5292

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants