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

ServiceSchema implementation #76

Merged
merged 13 commits into from
Apr 17, 2024
Merged

ServiceSchema implementation #76

merged 13 commits into from
Apr 17, 2024

Conversation

Monkatraz
Copy link
Contributor

@Monkatraz Monkatraz commented Apr 3, 2024

TypeScript has nearly taken my soul.

This PR replaces our current way of creating services. You now create them like this:

const MyServiceSchema = ServiceSchema.define(
  () => ({ count: 0 }),
  {
    increment: Procedure.rpc({
      input: Type.Object({ amount: Type.Number() }),
      output: Type.Object({ current: Type.Number() }),
      async handler(ctx, input) {
        ctx.state.count += input.amount;
        return Ok({ current: ctx.state.count });
      }
    }),
  },
);

or like this, if you have no state:

const MyServiceSchema = ServiceSchema.define({
  add: Procedure.rpc({
    input: Type.Object({ a: Type.Number(), b: Type.Number() }),
    output: Type.Object({ result: Type.Number() }),
    async handler(ctx, input) {
      return Ok({ result: input.a + input.b });
    }
  }),
});

Either way, you're given back a ServiceSchema, which you can basically treat like a class for your service. Services also no longer have names, instead you provide a dictionary to the server where the keys are the names, and the values are the service schemas:

const server = createServer(transport, { myService: MyServiceSchema })

This way of constructing services gets us 90% out of recursive TypeScript hell, and is also intended to make writing services easier, primarily through better legibility.

Some other changes:

  • To be both more accurate and to avoid type issues related to never, procedures can now always emit RiverUncaughtSchema errors and it is now allowed to omit the errors property when creating procedures. If you don't provide the prop, it just defaults to Type.Never().
  • Procedures now must be constructed via the Procedure object, e.g. Procedure.rpc(...). This prevents footguns that can happen by manually providing procedures to services (TS gets confused and doesn't type check your procedure!), and also lets us enable ergonomics (like omitting the errors prop). The way the footgun is forcibly avoided is purely through the type system - procedures created by these constructors are "branded" with an inaccessible unique symbol (which is not actually there), and ServiceSchema.define accepts only branded procedures.

@Monkatraz Monkatraz changed the base branch from brn-separate-procedures to main April 4, 2024 16:59
@Monkatraz Monkatraz changed the title [VERY WIP] ServiceSchema implementation ServiceSchema implementation Apr 4, 2024
@Monkatraz Monkatraz marked this pull request as ready for review April 4, 2024 19:55
__tests__/fixtures/services.ts Outdated Show resolved Hide resolved
router/index.ts Show resolved Hide resolved
router/procedures.ts Show resolved Hide resolved
router/procedures.ts Show resolved Hide resolved
router/procedures.ts Show resolved Hide resolved
router/result.ts Show resolved Hide resolved
router/server.ts Show resolved Hide resolved
router/server.ts Outdated Show resolved Hide resolved
router/services.ts Outdated Show resolved Hide resolved
/**
* Serializes this schema's procedures into a plain object that is JSON compatible.
*/
serialize(): object {
Copy link
Member

Choose a reason for hiding this comment

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

maybe for another PR but i'd like to have a helper type that reconstructs ServiceSchemaMap from a bunch of serialized services

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would the state be serialized? Do we even care about state ever being serialized?

Copy link
Member

@jackyzha0 jackyzha0 Apr 10, 2024

Choose a reason for hiding this comment

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

i think ok for state to not be serialized, i meant for the client type (the result of Server<typeof serviceDefs>> normally

mostly to help us ensure we have a way to go from JSON schema generated by a non-TS implementation of river (e.g. Python or Protobufs ala AI chat) to something we can consume in TS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh okay I get it, if you have a literal serialized state (in types) it lets you convert to (approximately) the service schema map that was serialized, albeit without state info

Copy link
Member

@jackyzha0 jackyzha0 left a comment

Choose a reason for hiding this comment

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

looks good! can we bump to 0.16.0 before merging?

@@ -251,18 +298,30 @@ export class ServiceSchema<
>;
// actual implementation
static define(
Copy link
Member

Choose a reason for hiding this comment

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

i wonder if, instead of overloading, we should just have define and defineWithConfig and have define just call defineWithConfig with the default config { initializeState: () => ({}) }

@Monkatraz Monkatraz merged commit 9efd210 into main Apr 17, 2024
3 checks passed
@Monkatraz Monkatraz deleted the brn-service-schemas branch April 17, 2024 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants