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

Implement Effect.Service and allow multiple layers to be provided in Effect.provide #3690

Open
wants to merge 31 commits into
base: next-minor
Choose a base branch
from

Conversation

mikearnaldi
Copy link
Member

@mikearnaldi mikearnaldi commented Sep 26, 2024

Namely the following is now possible:

class Prefix extends Effect.Service<Prefix>()("Prefix", {
  sync: () => ({
    prefix: "PRE"
  })
}) {}

class Postfix extends Effect.Service<Postfix>()("Postfix", {
  sync: () => ({
    postfix: "POST"
  })
}) {}

const messages: Array<string> = []

class Logger extends Effect.Service<Logger>()("Logger", {
  accessors: true,
  effect: Effect.gen(function* () {
    const { prefix } = yield* Prefix
    const { postfix } = yield* Postfix
    return {
      info: (message: string) =>
        Effect.sync(() => {
          messages.push(`[${prefix}][${message}][${postfix}]`)
        })
    }
  }),
  dependencies: [Prefix.Default, Postfix.Default]
}) {}

describe("Effect", () => {
  it.effect("Service correctly wires dependencies", () =>
    Effect.gen(function* () {
      const { _tag } = yield* Logger
      expect(_tag).toEqual("Logger")
      yield* Logger.info("Ok")
      expect(messages).toEqual(["[PRE][Ok][POST]"])
      const { prefix } = yield* Prefix
      expect(prefix).toEqual("PRE")
      const { postfix } = yield* Postfix
      expect(postfix).toEqual("POST")
    }).pipe(Effect.provide([Logger.Default, Prefix.Default, Postfix.Default]))
  )
})

Copy link

changeset-bot bot commented Sep 26, 2024

🦋 Changeset detected

Latest commit: 3af7f31

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

This PR includes changesets to release 32 packages
Name Type
effect Minor
@effect/cli Major
@effect/cluster-browser Major
@effect/cluster-node Major
@effect/cluster-workflow Major
@effect/cluster Major
@effect/experimental Major
@effect/opentelemetry Major
@effect/platform-browser Major
@effect/platform-bun Major
@effect/platform-node-shared Major
@effect/platform-node Major
@effect/platform Major
@effect/printer-ansi Major
@effect/printer Major
@effect/rpc-http Major
@effect/rpc Major
@effect/schema Major
@effect/sql-d1 Major
@effect/sql-drizzle Major
@effect/sql-kysely Major
@effect/sql-libsql Major
@effect/sql-mssql Major
@effect/sql-mysql2 Major
@effect/sql-pg Major
@effect/sql-sqlite-bun Major
@effect/sql-sqlite-node Major
@effect/sql-sqlite-react-native Major
@effect/sql-sqlite-wasm Major
@effect/sql Major
@effect/typeclass Major
@effect/vitest Major

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

@tim-smart
Copy link
Member

Effect.Tag.effect/scoped/sync could be a good option too.

@patroza
Copy link
Member

patroza commented Sep 26, 2024

Yes this rocks!
getting closer to mine https://github.com/effect-ts-app/libs/blob/main/packages/core/src/Context.ts#L247
scoped would be great, mine exposes toLayer and toLayerScoped.
I would still expose a to variant here too so you can easily build e.g Test or Fake layers too?

@mikearnaldi
Copy link
Member Author

I would still expose a to variant here too so you can easily build e.g Test or Fake layers too?

class DateTag extends Effect.Tag("DateTag", {
  sync: () => new Date(1970, 1, 1)
})<DateTag>() {
  static Test = Layer.sync(this, () => new Date(1970, 1, 2))
}

not sure I understand

@patroza
Copy link
Member

patroza commented Sep 26, 2024

class DateTag extends Effect.Tag("DateTag", {
  sync: () => new Date(1970, 1, 1)
})<DateTag>() {
  static Test = Layer.sync(this, () => new Date(1970, 1, 2))
}

not sure I understand

Yea it's fine too. not sure why I made .toLayer() using it's make and .toLayer(someTestEffect) to e.g do a test layer

@mikearnaldi
Copy link
Member Author

Updated:

Namely the following is now possible:

import { Effect } from "effect"

class Logger extends Effect.Tag("Logger", {
  sync: () => ({
    info: (message: string) => Effect.logInfo(message)
  })
})<Logger>() {}

Together with:

import { Effect } from "effect"

class Logger extends Effect.Tag("Logger", {
  effect: Effect.sync(() => ({
    info: (message: string) => Effect.logInfo(message)
  }))
})<Logger>() {}

And:

import { Effect } from "effect"

class Logger extends Effect.Tag("Logger", {
  scoped: Effect.gen(function* () {
    yield* Effect.addFinalizer(() => Effect.logInfo("Done"))
    return {
      info: (message: string) => Effect.logInfo(message)
    }
  })
})<Logger>() {}

All the above will lead to automatic layer creation named: Logger so you can directly do Effect.provide(Logger).

The created layer is also exported with the name Logger.Layer to expose non packed dependencies for testing:

import { Effect, Layer } from "effect"

class Dummy extends Effect.Tag("Dummy", {
  sync: () => ({})
})<Dummy>() {}

class Logger extends Effect.Tag("Logger", {
  effect: Effect.gen(function* () {
    yield* Dummy
    return {
      info: (message: string) => Effect.logInfo(message)
    }
  }),
  dependencies: [Dummy]
})<Logger>() {}

You'll have that Logger is Layer<Logger, never, never> while Logger.Layer is Layer<Logger, never, Dummy>.

@mikearnaldi
Copy link
Member Author

If we make Effect.provide variadic (with auto-merging) we are basically hiding layers almost fully, Effect.provide(Logger, Db, ..)

@patroza
Copy link
Member

patroza commented Sep 26, 2024

You'll have that Logger is Layer<Logger, never, never> while Logger.Layer is Layer<Logger, never, Dummy>.

Is kind of neat yes. maybe call it liveDependencies, not sure. Maybe it's fine as is as Live is considered the default.

You'll have that Logger

Do you mean typeof Logger (which is also the Tag).
In my implementation, Logger is the opaque service interface.

@mikearnaldi
Copy link
Member Author

Do you mean typeof Logger (which is also the Tag).

yes

In my implementation, Logger is the opaque service interface.

that won't be implemented here as you'll fill up your service with irrelevant fields and lose assignability to plain objects

@patroza
Copy link
Member

patroza commented Sep 26, 2024

that won't be implemented here as you'll fill up your service with irrelevant fields and lose assignability to plain objects

well, I was also the one fully against that when we were discussing the Context, and making it a requirement for service interfaces to be polluted with said fields, but now that it's optional, an implementation detail of your tag/service,
I am happy with it for almost all my services.
I expose: static of = (service: Omit<Id, keyof Context.TagClassShape<any, any>>)) => Id aka static of = (service: ServiceShape) => Id, which is just implemented as identity, as they are virtual fields anyway.

@mikearnaldi
Copy link
Member Author

that won't be implemented here as you'll fill up your service with irrelevant fields and lose assignability to plain objects

well, I was also the one fully against that when we were discussing the Context, and making it a requirement for service interfaces to be polluted with said fields, but now that it's optional, an implementation detail of your tag/service, I am happy with it for almost all my services. I expose: static of = (service: Omit<Id, keyof Context.TagClassShape<any, any>>)) => Id aka static of = (service: ServiceShape) => Id, which is just implemented as identity, as they are virtual fields anyway.

but the pollution isn't only on assign, it's also on usage, you'll have stuff like _op, etc in your service when you consume it. Anyway your code your decision :)

@patroza
Copy link
Member

patroza commented Sep 26, 2024

but the pollution isn't only on assign, it's also on usage, you'll have stuff like _op, etc in your service when you consume it.

No we're conflatingLogger with typeof Logger
typeof Logger, has the op/tag/Layer, Id is clean right, only has TagClassShape,
but yes you're right, Type and Id show up:
Screenshot 2024-09-26 at 16 15 06
but those could be similarly hidden under a symbol like [TagTypeId].. but yea I get your point :)

Anyway your code your decision :)

of course

@patroza
Copy link
Member

patroza commented Sep 26, 2024

Oh it used to be that way when I implemented mine
IMG_5493

@mikearnaldi
Copy link
Member Author

but the pollution isn't only on assign, it's also on usage, you'll have stuff like _op, etc in your service when you consume it.

No we're conflatingLogger with typeof Logger typeof Logger, has the op/tag/Layer, Id is clean right, only has TagClassShape, but yes you're right, Type and Id show up: Screenshot 2024-09-26 at 16 15 06 but those could be similarly hidden under a symbol like [TagTypeId].. but yea I get your point :)

Anyway your code your decision :)

of course

why the abstract?

@mikearnaldi mikearnaldi marked this pull request as draft September 26, 2024 14:34
@patroza
Copy link
Member

patroza commented Sep 26, 2024

why the abstract?

leftover I think, I guess because using the constructor in userland wasn't really useful for anything, so abstract kinda tells you not to call new :D but otoh it begs "inherit me", so not sure we had a net win here.

@mikearnaldi
Copy link
Member Author

why the abstract?

leftover I think, I guess because using the constructor in userland wasn't really useful for anything, so abstract kinda tells you not to call new :D but otoh it begs "inherit me", so not sure we had a net win here.

also how did you manage to override of from Tag?

@github-actions github-actions bot changed the base branch from main to next-minor September 26, 2024 15:27
@mikearnaldi
Copy link
Member Author

I think I am kind of happy with: e17d085

cc @patroza @tim-smart @schickling @IMax153

@IMax153
Copy link
Member

IMax153 commented Sep 26, 2024

I'm a fan of this - I especially like the idea of a variadic Effect.provide with auto-merging.

@mikearnaldi
Copy link
Member Author

I'm a fan of this - I especially like the idea of a variadic Effect.provide with auto-merging.

actually looking at implementation it won't be possible: Effect.provide(A, B, C, D) we can't know if A is an effect or a layer when it is a tag that is both an effect and a layer

@mikearnaldi
Copy link
Member Author

I guess b95558e is fine though, looks like:

describe("Effect", () => {
  it.effect("Service correctly wires dependencies", () =>
    Effect.gen(function*() {
      const { _tag } = yield* Logger
      expect(_tag).toEqual("Logger")
      yield* Logger.info("Ok")
      expect(messages).toEqual(["[PRE][Ok][POST]"])
      const { prefix } = yield* Prefix
      expect(prefix).toEqual("PRE")
      const { postfix } = yield* Postfix
      expect(postfix).toEqual("POST")
    }).pipe(
      Effect.provide([Logger, Prefix, Postfix])
    ))
})

@mikearnaldi
Copy link
Member Author

mikearnaldi commented Sep 26, 2024

The only remaining footgun is calling provide multiple times, wondering if we should work on having a shared global memoMap to avoid layer re-instantiation

cc @tim-smart

@mikearnaldi mikearnaldi changed the title Allow Effect.Tag to take a constructor, enables direct inference and layer construction. Implement Effect.Service and allow multiple layers to be provided in Effect.provide Sep 26, 2024
@patroza
Copy link
Member

patroza commented Oct 5, 2024

Still trouble when the service is exported, even with using the BrandId:
Screenshot 2024-10-05 at 16 14 53

'extends' clause of exported class 'Logger' has or is using private name 'BrandTypeId'.

In this case I typed proxy: true instead of accessors: true

@mikearnaldi
Copy link
Member Author

Still trouble when the service is exported, even with using the BrandId: Screenshot 2024-10-05 at 16 14 53

'extends' clause of exported class 'Logger' has or is using private name 'BrandTypeId'.

In this case I typed proxy: true instead of accessors: true

Is that the case even when the overload is correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Needs merge
Development

Successfully merging this pull request may close these issues.

4 participants