-
-
Notifications
You must be signed in to change notification settings - Fork 128
Effect beginner tutorial. #1046
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
base: feat/add-tutorials
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@samwho is attempting to deploy a commit to the Effect Team on Vercel. A member of the Team first needs to authorize it. |
content/src/content/docs/learn/tutorials/hooking-up-a-database.mdx
Outdated
Show resolved
Hide resolved
content/src/content/docs/learn/tutorials/hooking-up-a-database.mdx
Outdated
Show resolved
Hide resolved
content/src/content/docs/learn/tutorials/hooking-up-a-database.mdx
Outdated
Show resolved
Hide resolved
content/src/content/docs/learn/tutorials/hooking-up-a-database.mdx
Outdated
Show resolved
Hide resolved
content/src/content/docs/learn/tutorials/hooking-up-a-database.mdx
Outdated
Show resolved
Hide resolved
IMax153
left a comment
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.
Just a few minor comments for today! Great work @samwho - it's coming along very nicely!
content/src/content/docs/learn/tutorials/hooking-up-a-database.mdx
Outdated
Show resolved
Hide resolved
content/src/content/docs/learn/tutorials/hooking-up-a-database.mdx
Outdated
Show resolved
Hide resolved
content/src/content/docs/learn/tutorials/hooking-up-a-database.mdx
Outdated
Show resolved
Hide resolved
content/src/content/docs/learn/tutorials/hooking-up-a-database.mdx
Outdated
Show resolved
Hide resolved
….mdx Co-authored-by: Maxwell Brown <maxwellbrown1990@gmail.com>
….mdx Co-authored-by: Maxwell Brown <maxwellbrown1990@gmail.com>
| With this information we can ensure, and get TypeScript to verify for us, | ||
| that things do not go wrong in unexpected ways. | ||
|
|
||
| ```ts twoslash | ||
| // @errors: 2375 | ||
| import { Effect } from "effect" | ||
|
|
||
| const effect = Effect.succeed(1).pipe( | ||
| Effect.tryMap({ | ||
| try: (n) => n + 1, | ||
| catch: () => new Error(), | ||
| }), | ||
| ) | ||
|
|
||
| const noError: Effect.Effect<number, never, never> = effect |
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.
This transition felt a bit abrupt to me. To me, it might feel more natural if you insert one of the "aside"-esque comments you've had previously (recall the pipe aside). Perhaps "An aside on errors in Effect" or something?
Also, before now Effect.tryMap has not even been mentioned but now we're using it all of a sudden.
Might be worth using the user fetch code from above for these samples instead of the random tryMap code, given that you also insert it into the final snippet later anyways.
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 did wonder about whether to explicitly call out Effect.tryMap and landed on not doing so. It felt like because we've already covered Effect.map, and we looked at Effect.try in the previous section, the reader wouldn't flinch at Effect.tryMap being casually used without explanation.
A small nod to it might be the way to go, though. I need to start being careful, I've not spent more time with Effect than the reader and am likely prone to thinking they'll understand things they might not. 😅
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.
My take reading through this is that this section would honestly be more clear to me if you just stick with the chunk of code from before:
Effect.tryPromise({
try: () => db.query("SELECT * FROM users"),
catch: (cause) => new UserFetchError("failed to fetch users", { cause }),
})| This effectively recreates the behaviour of throwing an exception. Defects are | ||
| critical errors that you don't want to handle. If you were to create a defect in | ||
| an Effect handler, the result would be a 500 error returned to the user. |
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.
Might be worth one more sentence or something here providing a clear distinction between "expected" errors, which are part of the program's domain, and "defects" which are critical errors (as you explained).
| //---cut--- | ||
| class UserFetchError extends Error {} | ||
|
|
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.
For a variety of reasons, I would advise against this pattern.
For one, the lack of a discriminator in the type (e.g. a _tag) means that another error which extends Error can be directly assignable to the UserFetchError: https://effect.website/play#55cd56ec464d.
My recommendation would be to either:
- Take the time to introduce creating your own domain-specific errors more properly here via
Data.TaggedErrorhttps://effect.website/docs/data-types/data/#taggederror - Just make the error an interface in this case for now if you want to keep it simple
interface UserFetchError {
readonly _tag: "UserFetchError"
readonly message: string
readonly cause: unknown
}I personally think it might be good to go for the first option given that Data.TaggedError will automatically attach a _tag to our error type and also extends Error for us by default, but I leave that decision to you.
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 agonised over this, because tags do feel like they deserve special attention and I'll soon get to them when I talk about services. I wanted to wait for a place I needed to use them in error handling to introduce them. I also don't want to wander too far away from the purpose of this section: to migrate an endpoint that calls an external API.
This section in its entirety is still very early, probably the earliest I've pushed a section up for review. I'm not tightly committed to any part of it. If you think we really should introduce tags here, I don't think it would be impossible to do so. I do think we'll have to cut something else to make room for them, though, and it might have to be services.
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.
Totally understand, and honestly I don't want to impose my opinion too much on the direction you take here.
My only advice would be that if you want to avoid introducing Data.TaggedError, etc., maybe just add a _tag property or something to your custom error type and do another one of those "this is important but we'll discuss it later" things. Just to ensure you have a discriminator for the type in place, and you don't create an unintentional footgun with multiple error types that are assignable to one another.
IMax153
left a comment
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.
Leaving my feedback on Providing Requirements onward off this review for now, as discussed in Discord.
| const health = Effect.sync(() => { | ||
| return HttpServerResponse.text("ok") | ||
| }) |
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.
| const health = Effect.sync(() => { | |
| return HttpServerResponse.text("ok") | |
| }) | |
| const health = HttpServerResponse.text("ok") |
I still think we should get rid of the Effect.sync wrapper here. It's superfluous since the code works without it, and adds one more thing you have to explain.
| ```ts twoslash showLineNumbers=false | ||
| import { Effect } from "effect" | ||
| // ---cut--- | ||
| const syncResult = Effect.runSync( | ||
| Effect.sync(() => "Hello, world!") | ||
| ) | ||
| console.log(syncResult) | ||
| // => "Hello, world!" | ||
|
|
||
| const asyncResult = await Effect.runPromise( | ||
| Effect.promise(async () => "Hello, world!") | ||
| ) | ||
| console.log(asyncResult) | ||
| // => "Hello, world!" | ||
| ``` |
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.
Despite the commentary we received, I would advocate strongly against showing Effect.runSync at all. A user shouldn't care at all about function coloring. Any Effect can be run with Effect.runPromise, and we should probably just stick to this during the tutorial.
| This makes use of JavaScript's [generator functions][1]. The way Effect uses | ||
| them, if you think of an Effect as being like a `Promise`, `yield*`ing an | ||
| `Effect` is the same as `await`ing a `Promise`. Under the hood, all `Effect`s | ||
| are generators, and `yield*`ing one passes it to the Effect runtime and waits | ||
| for the result. |
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.
| This makes use of JavaScript's [generator functions][1]. The way Effect uses | |
| them, if you think of an Effect as being like a `Promise`, `yield*`ing an | |
| `Effect` is the same as `await`ing a `Promise`. Under the hood, all `Effect`s | |
| are generators, and `yield*`ing one passes it to the Effect runtime and waits | |
| for the result. | |
| This makes use of JavaScript's [generator functions][1]. If you think of an Effect as similar to a `Promise`, `yield*`ing an `Effect` is the same as `await`ing a `Promise`. Under the hood, all `Effect`s are generators, and `yield*`ing one passes it to the Effect runtime and waits for the result. |
| const effect = Effect.sync(() => "Hello, world!").pipe( | ||
| Effect.map((s) => s.toUpperCase()) | ||
| ) | ||
| const result = await Effect.runSync(effect) |
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.
| const result = await Effect.runSync(effect) | |
| const result = await Effect.runPromise(effect) |
|
|
||
| `HttpRouter` is a data structure that represents a collection of routes. The | ||
| simplest router is one with no routes at all, and Effect exposes that to us as | ||
| the `HttpRouter.empty` value. Under the hood, this is itself an `Effect`: |
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.
| the `HttpRouter.empty` value. Under the hood, this is itself an `Effect`: | |
| the `HttpRouter.empty` value. Under the hood, an `HttpRouter` is itself an `Effect`: |
| If we add a few more routes, we can see how this gets quickly out of hand: | ||
|
|
||
| ```ts twoslash showLineNumbers=false | ||
| import { HttpRouter, HttpServerResponse } from "@effect/platform" | ||
| import { Effect } from "effect" | ||
| const health = Effect.sync(() => { | ||
| return HttpServerResponse.text("ok") | ||
| }) | ||
| const status = Effect.sync(() => { | ||
| return HttpServerResponse.text("ok") | ||
| }) | ||
| const version = Effect.sync(() => { | ||
| return HttpServerResponse.text("ok") | ||
| }) | ||
| // ---cut--- | ||
| HttpRouter.get("/health", health)( | ||
| HttpRouter.get("/status", status)( | ||
| HttpRouter.get("/version", version)( | ||
| HttpRouter.empty, | ||
| ), | ||
| ), | ||
| ) | ||
|
|
||
| // vs | ||
|
|
||
| HttpRouter.empty.pipe( | ||
| HttpRouter.get("/version", version), | ||
| HttpRouter.get("/status", status), | ||
| HttpRouter.get("/health", health), | ||
| ) | ||
| ``` |
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.
Suggest two snippets here. The first one could leads with "... see how this gets out of hand..." and the second leads with something like "versus using pipe":
| For example, [Neon](https://neondb.com/) offers a free tier that should be | ||
| more than enough for this tutorial. They also offer a web-based UI for creating | ||
| tables and rows. |
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.
While I understand wanting to point users in the right direction, I'm not sure we should direct them to a specific hosted service.
| ) | ||
|
|
||
| const main = Effect.gen(function* () { | ||
| yield* Effect.promise(() => db.connect()) |
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.
| yield* Effect.promise(() => db.connect()) | |
| // Make sure we connect to the database before starting the server | |
| yield* Effect.promise(() => db.connect()) |
Might be worth adding a comment just for this snippet since user's might be a bit confused why this is required? Idk
|
|
||
| An Effect that returns an `HttpServerResponse`, may throw an `HttpBodyError`, | ||
| but also, for the first time, has something other than `never` in that third | ||
| spot. As a reminder, this is the `Requirement` type. What it's saying here is |
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.
| spot. As a reminder, this is the `Requirement` type. What it's saying here is | |
| spot. As a reminder, this is the `Requirements` type. What it's saying here is |
content/src/content/docs/learn/tutorials/services-and-config.mdx
Outdated
Show resolved
Hide resolved
Co-authored-by: Maxwell Brown <maxwellbrown1990@gmail.com>
Type
Description
Currently still a work in progress.