-
-
Notifications
You must be signed in to change notification settings - Fork 231
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
Use literal strings for service tags #1955
Conversation
🦋 Changeset detectedLatest commit: 632cdac The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
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 |
I suppose this was one way of getting rid of some of the X.X :D bravo! will need to play with it for a bit for more meaningful feedback :) |
a96652f
to
8ae8507
Compare
While name collision is a thing. Do we consider it a feature or a downgrade, that you will no longer be warned when you have multiple versions of libraries? |
I definitely favor this overall idea. My app at work mostly uses string-literal identifiers, it's honestly easier to read most of the time in the Context parameter, and I utilize I dunno if it's just familiarity, but I honestly prefer the order of parameters the opposite direction const a = Tag("Id")
const b = a<number>()
const c = a<string>() while this does make sense const a = Tag<number>()
const b = a("foo")
const c = a("bar") Not something that I think Effect needs, it's fairly opinionated on my end, but I have a Context library that builds atop of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it, it simplifies a few things.
I agree that the arg order should switch.
@@ -38,27 +37,27 @@ export const serve = dual< | |||
(): <R, E>( | |||
httpApp: App.Default<R, E> | |||
) => Layer.Layer< | |||
Server.Server | Exclude<R, ServerRequest.ServerRequest | Scope.Scope>, | |||
Server.Server | Exclude<R, ServerRequest.ServerRequest | "Scope">, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few updates needed in this file
readonly tag: Context.Tag<SchemaStore<A>, SchemaStore<A>> | ||
readonly layer: Layer.Layer<KeyValueStore, never, SchemaStore<A>> | ||
} = internal.layerSchema | ||
export const layerSchema: <I, A, K extends string>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely improves the service factory pattern.
I've been thinking about this a bit more, and I'm less sure this really is something we want to force upon users. I'd say requiring the string literals to make them global and identifiable could be a good thing still, but less sure about them being forced as the identifiers in the Context type param. While I use them in my apps quite a lot, I think the key difference there is I rely almost exclusively on inference there except in more rare cases of recursion. However, in the case of the "core" services that the effect package itself offers, there's a LOT of times where I create library code which explicitly mention them so type-inference is correct and/or not infinitely deep. In those circumstances I quite like only needing to import, say I feel like I can't articulate it fully, but I also feel like there's going to be some annoyance with refactoring these string literals as they won't be connected to any specific TS symbol without some more convention, maybe something like this export interface Scope { ... }
// Don't love this convention because now typing "Tag" will autocomplete lots and lots of packages
export const Tag = Context.Tag<Scope>()("Scope")
export type Tag = Context.Tag.Identifier<typeof Tag> Maybe these aren't so bad to live with, but I figured I'd at least share some "cons" of this approach for more discussion before we get too far. |
This change won't include the renaming of tags, so the Scope tag will still be called But I agree the biggest con of this change is losing jump to definition for identifiers - but it is something I hardly ever do myself. |
07c1921
to
40aadfb
Compare
@tim-smart I don't too often either, but I think we've likely written, touched, or at least read all of the services we'd ever possibly utilize. This isn't true for people just getting started though, I'm actually onboarding a new team member, and I can say "following the TS types" is a pretty common pattern for learning our codebase |
I still use jump to definition for the tags themselves, and on the actual service implementation type, but rarely for tag identifiers. We are only losing one of the three here, so the impact isn't massive in my opinion. |
The convenience now is that they're intrinsically linked for all the Effect services. It changes after this lands though, finding the tag is all that much harder. Take the following, contrived for brevity, example import * as Effect from 'effect/Effect'
function foo<R, E, A>(effect: Effect.Effect<R, E, A>): Effect.Effect<"Scope" | R , E, A> {
return Effect.gen(function*(_) {
yield* _(Effect.addFinalizer(someFinalizer))
return yield* _(effect)
})
} There's 0 reference to the Scope Tag or Service, only the identifier. This function no longer has any connection to Then if you're actually curious about what the heck a Scope is you might hover over
And you still won't have any idea what/where Scope comes from. You might follow /* @internal */
export const addFinalizer = <R, X>(
finalizer: (exit: Exit.Exit<unknown, unknown>) => Effect.Effect<R, never, X>
): Effect.Effect<R | "Scope", never, void> =>
core.withFiberRuntime(
(runtime) => {
const acquireRefs = runtime.getFiberRefs()
const acquireFlags = runtime._runtimeFlags
return core.flatMap(scope, (scope) =>
core.scopeAddFinalizerExit(scope, (exit) =>
core.withFiberRuntime((runtimeFinalizer) => {
const preRefs = runtimeFinalizer.getFiberRefs()
const preFlags = runtimeFinalizer._runtimeFlags
const patchRefs = FiberRefsPatch.diff(preRefs, acquireRefs)
const patchFlags = _runtimeFlags.diff(preFlags, acquireFlags)
const inverseRefs = FiberRefsPatch.diff(acquireRefs, preRefs)
runtimeFinalizer.setFiberRefs(
FiberRefsPatch.patch(patchRefs, runtimeFinalizer.id(), acquireRefs)
)
return ensuring(
core.withRuntimeFlags(finalizer(exit) as Effect.Effect<never, never, X>, patchFlags),
core.sync(() => {
runtimeFinalizer.setFiberRefs(
FiberRefsPatch.patch(inverseRefs, runtimeFinalizer.id(), runtimeFinalizer.getFiberRefs())
)
})
)
})))
}
) Now you're forced to read this internal implementation to find the I definitely find this subpar for someone who learns using the TS types. Especially as the ecosystem builds up more and more and there will only be larger layers of indirection between the API a developer is trying to learn and the Effect services they'll inevitably utilize |
I think Scope isn't a great example, as even now the jump to definition for the identifier / service isn't great and you have to rely on the jsdoc to understand what a function does. I agree that losing jump to definition for service identifiers isn't great, but it is a small loss considering the gains in other areas. |
Not sure that this was a feature, version checks shouldn't really be lib level, duplicate checks should be done at install level |
Not sure why a user should care about where the Scope tag comes from, it's just an "internal" from the user perspective, in the Effect module you have some functions that require "Scope" and some that provides "Scope" like Curious if we can work around this via lsp |
Would |
c057f9a
to
62da5e9
Compare
Superseded by: #2028 |
Requesting early review for feedback before refactoring the rest.
This PR standardises tags to use string literals, basically a service is now represented by a tag like:
and will be identified with the literal
"UseRepo"
in types.This solves in one shot multiple issues, assignability of potentially similar types (uniqueness at the type level) and automatically makes tags global, meaning instantiating multiple times the same tag will lead to the same instance.
Debuggability also gains from having human readable names for every service in context and arguably the types are overall much simpler.
There is a degree of awkwardness in layers that now look like
Layer<"Database", never, "UserRepo" | "TodoRepo">
but I quite like it