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

add serviceEffects and change serviceConstants semantic (fixes #634) #659

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sledorze
Copy link
Contributor

@sledorze sledorze commented Sep 8, 2023

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Sep 8, 2023

🦋 Changeset detected

Latest commit: 48df8f4

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

@sledorze sledorze self-assigned this Sep 8, 2023
@@ -1842,6 +1842,24 @@ export const serviceFunctions = <I, S>(
/** @internal */
export const serviceConstants = <I, S>(
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking we simplify this to:

/**
 * @since 1.0.0
 * @category context
 */
export const serviceConstants: <I, S>(tag: Context.Tag<I, S>) => { [K in keyof S]: Effect<I, never, S[K]> } =
  effect.serviceConstants

Then we won't need to add the same function to the other ecosystem packages.

Copy link
Member

Choose a reason for hiding this comment

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

Also, serviceFunctions could be:

/**
 * @since 1.0.0
 * @category context
 */
export const serviceFunctions: <I, S>(
  tag: Context.Tag<I, S>
) => {
  [K in { [K in keyof S]: S[K] extends (...args: Array<any>) => any ? K : never }[keyof S]]: [S[K]] extends
    [(...args: infer Args) => infer R]
    ? R extends Effect<infer R, infer E, infer A> ? (...args: Args) => Effect<I | R, E, A>
    : (...args: Args) => Effect<I, never, R>
    : never
} = effect.serviceFunctions

Which would also support non-effect returning functions.

Copy link
Member

Choose a reason for hiding this comment

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

we can't merge the two because we can't distinguish () => any with () => Effect, we'd need to check the return type and always flatten when it's an effect but that would end up flattening also the case where for example the return value is an option or an effect that shouldn't be flattened

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also remember that implementation wise, we can't know the nature of the returning type at value level

Copy link
Member

@tim-smart tim-smart Sep 15, 2023

Choose a reason for hiding this comment

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

for example the return value is an option or an effect that shouldn't be flattened

I think this is already an issue due to the type augmentation.

But it does introduce an expectation gap, so yes I think we should not do it. Just make the constants change.

I had a go at the implementation here: 5da6a97

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ofc core.isEffect ;)

@mikearnaldi
Copy link
Member

mikearnaldi commented Sep 15, 2023 via email

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.

3 participants