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

Q: How does one setConfigProvider globally? #14

Closed
vecerek opened this issue Aug 14, 2023 · 13 comments
Closed

Q: How does one setConfigProvider globally? #14

vecerek opened this issue Aug 14, 2023 · 13 comments

Comments

@vecerek
Copy link
Contributor

vecerek commented Aug 14, 2023

When one has an Effect, they can change the default config provider like so:

Effect.succeed(42),
Effect.provideLayer(Effect.setConfigProvider(ConfigProvider.fromMap(new Map([["key", "value"]]))))

However, I don't see how I can do this in the context of this plugin. If I add it to the effect resolver, it messes up my types because all the layers need to be provided before I can set a new config provider. I cannot do it in the effect.layers option either for a similar reason, and when I add that to the globalLayer effect options like so:

effectOptions: {
  globalLayer: Layer.provide(
    Effect.setConfigProvider(ConfigProvider.fromMap(new Map([["key", "value"]]))),
    Layer.context()
  ),
},

it type checks but does not seem to work at all. @iamchanii any idea how to configure your own config provider when using this plugin? 🙏

@iamchanii
Copy link
Owner

It looks ugly but working.

  builder = new SchemaBuilder<SchemaTypes>({
    effectOptions: {
      globalLayer: Layer.provide(
        Effect.setConfigProvider(ConfigProvider.fromMap(new Map([['HOST', 'localhost']]))),
        Layer.context(),
      ),
    },
    plugins: [EffectPlugin, RelayPlugin],
    relayOptions: {},
  });

  builder.queryType({});

  builder.queryField('roll', t =>
    t.effect({
      resolve() {
        return pipe(
          Effect.config(Config.withDefault(Config.string('HOST'), '0.0.0.0')),
          Effect.flatMap(host => Effect.succeed(host)),
          // What should I do for ConfigError?
          Effect.catchAll(() => Effect.succeed('none')),
        );
      },
      type: 'String',
    }));

  const schema = builder.toSchema();
  const document = parse(`{ roll }`);
  const result = await execute({ document, schema });

  expect(result.data).toEqual({
    roll: 'localhost',
  });

What does it type checks but does not seem to work at all meaning? I just feeling I misunderstood.

@vecerek
Copy link
Contributor Author

vecerek commented Aug 15, 2023

@iamchanii I have a bit more complex setup than your example. I tried your example and it sort of works. In my case, I am providing a layer to the effect field in its configuration. That layer depends on another layer, which depends on an another layer until the very last layer actually depends on a config. For some reason, I'm getting an error like this:

(Missing data at /secrets.MY_SECRET: "Expected /secrets_MY_SECRET to exist in the process context")

However, I'm providing a config provider from a map and therefore I'm expecting to see something more like this:

(Missing data at /secrets.MY_SECRET: "Expected /secrets.MY_SECRET to exist in the provided map")

This is confusing to me but irrelevant for the big picture. I'll try to water my use case down to a minimal reproducible code. However, my bigger concern is the following.

I would like this error to happen at deploy time instead of at runtime, i.e. when resolving the GraphQL requests. I want my application to fail the deployment if some configuration is missing. I'm not sure how this could be achieved, or if at all in this plugin but it's a crucial requirement I have nonetheless. The problem I see is that pothos-plugin-effect creates an effect runtime on every request. Somehow, I'd need that runtime to be built ahead of time and the resolution of all graphql requests would need to live within that single runtime. That would solve my issue, I believe. I just don't know how feasible that is.

@iamchanii
Copy link
Owner

I would like this error to happen at deploy time instead of at runtime, i.e. when resolving the GraphQL requests. I want my application to fail the deployment if some configuration is missing

In fact, there is very basic type inferrence for effect layers, contexts and services. getting layers as typed tuple and typing it is little bit difficult (at least for me) but I think this library have to do it.

The problem I see is that pothos-plugin-effect creates an effect runtime on every request. Somehow, I'd need that runtime to be built ahead of time and the resolution of all graphql requests would need to live within that single runtime.

Interest. How about this api?

effectOptions: {
  customRuntime: () => {
    return Runtime.make(...);
  };
}

@vecerek
Copy link
Contributor Author

vecerek commented Sep 17, 2023

Hey @iamchanii 👋 sorry for coming back to you so late. I was thinking about this plugin a lot these past few weeks. The whole layer thing didn't work for me conceptually. I believe it is an API design mistake to surface the construction of layers through the field definitions. In Effect, it is important that these layers get constructed when the application starts as opposed to at runtime. As an application author, I do not want to handle ConfigErrors at runtime. I want the deployment of my application to fail if the configuration is wrong or missing. Otherwise, such failures could lead to serious production incidents which could have been easily avoided.

To figure out what the core value of this plugin is, I decided to remove it from my codebase and try to use some minimal implementation. This is what I ended up with (it's just pseudo-code, don't mind if it doesn't run):

When I initialize my app, I create an Effect runtime which I pass to the graphql resolvers via a modified context object.

// index.ts
const main = async () => {
  const runtime = await makeRuntime(); // imagine this returns an Effect runtime

  const yoga = createYoga({
    schema,
    (context) => ({ ...context, runtime }),
  });

  const server = createServer(yoga);

  server.listen(4000, () => {
    console.info('Server is running on http://localhost:4000/graphql');
  });
}

void main().catch(console.error);

I also created what I think the core behavior of this library is, a graphql effect resolver:

export const resolveEffect = <R>(
  runtime: Runtime.Runtime<R>
) => async <E, A>(
  effect: Effect.Effect<R, E, A>
) => {
  const exit = await Runtime.runPromiseExit(runtime)(effect);

  if (Exit.isFailure(exit)) {
    const cause = Cause.unannotate(exit.cause);

    if (Cause.isFailType(cause) && cause.error instanceof Error) {
      throw cause.error;
    }

    throw new Error(Cause.pretty(cause));
  }

  return exit.value;
}

And then, all I need to do is use it in my resolver like so:

resolver: (_parent, _args, context) => pipe(
  // imagine there's an Effect here,
  resolveEffect(context.runtime)
)

Having reflected on this issue, I've come to the conclusion that this plugin should do two things:

  1. remove all mentions of layers to prevent its users from shooting themselves in the foot,
  2. allow the user to pass in a reference to a runtime, not a runtime construction function, to make sure that this runtime is built at application startup time, not during the handling of a request. It should be a single runtime reference passed to all resolvers. Then the plugin could do the effect resolution for me as it does today but using the runtime if one was provided.

What do you think @iamchanii?

@iamchanii
Copy link
Owner

iamchanii commented Sep 17, 2023

@vecerek I'm so appreciate for your feedback! first of all, this library started from just small idea so there may be design mistakes. but I think we can still take it right way. and then, we can release first major release.

I'm not sure if you know, recently I worked about refactoring and writing some documentations. and I got some thoughts as similar as you thought. Is really need to define services, contexts and layers in object field? and I am not experienced about Effect, but in general, I think it's enoguth to have a "main" Layer to context management. (And, hmm. I still think field-level context management maybe usefull some use cases nevertheless)

remove all mentions of layers to prevent its users from shooting themselves in the foot,

👍

Is this meaning for field-level's mentions of layers? just applied for layers? how about contexts and services?

allow the user to pass in a reference to a runtime, not a runtime construction function, to make sure that this runtime is built at application startup time, not during the handling of a request. It should be a single runtime reference passed to all resolvers. Then the plugin could do the effect resolution for me as it does today but using the runtime if one was provided.

👍

like so:

// $ExpectType Runtime.Runtime<...>
const runtime = await makeEffectRuntime();

const builder = new SchemaBuilder<{
  EffectRuntime: typeof runtime
  // or
  // EffectRuntime: Runtime.Runtime<...>
}>({
  plugins: [EffectPlugin],
  effectOptions: { runtime },
});

I think this is more simple than globalLayer and globalContext and this could be replaced these features. but I concerned that all users of this plugin will require knowledges about Runtime and make it. how do you think about it?

@mikearnaldi
Copy link

There seems to be some confusion around context/layer/services, they are different things:

  1. a Layer is Constructor of Context, a Layer should only be built at boot time
  2. a Context is an Object that carries a set of services
  3. a Service is a value contained in a Context object

Generally speaking "field-level" provision of layer / context is useless, the sole reason to use context is to let the context bubble up through your app and provide it at the top, putting in the same place access of context and provision of context defeats the purpose of using context in the first place, a user could simply do whateverEffect.pipe(Effect.provide(context)) by themselves, no new api needed.

You really have only two choices:

  • you control initialisation and expose it as an effect itself so your final composed schema exposes some kind of "bindApp: Effect<ContextFromSchema, never, void>" and you internally run an Effect.runtime() during init to get a runtime to use when resolving queries (that carries context)
  • you don't control initialisation and rely on a user giving you a runtime that you use to execute

In a general sense to use Effect you need to know about Runtime so I wouldn't worry about a user that wants to use Effect but doesn't know Runtime

@vecerek
Copy link
Contributor Author

vecerek commented Oct 25, 2023

I fully agree with @mikearnaldi. Because of the design of pothos, controlling the initialization of the app is not achievable. Our only option is:

you don't control initialisation and rely on a user giving you a runtime that you use to execute

And about

but I concerned that all users of this plugin will require knowledges about Runtime and make it. how do you think about it?

I don't think Runtime is a hard-to-grasp concept. Also, if users already have a Layer (could be one that was created by merging many layers), creating a Runtime from that layer is as simple as applying Layer.toRuntime. Providing some code examples that show how runtimes can be created should be enough to address your concerns.

@iamchanii
Copy link
Owner

@mikearnaldi @vecerek thank you for your comments! I'm still learning about Effect, so I don't have it all figured out, but I'll share some thoughts on the plugin's implementation soon.

@iamchanii
Copy link
Owner

iamchanii commented Oct 25, 2023

I just want sync my thoughts, and want to get a review from you.

  1. Remove effect options from t.effect. and then, I would be able to remove many lines of type inference related code.

    t.effect({
      type: 'String',
    - effect: {
    -   layers: [/* ... */],
    -   contexts: [/* ... */],
    -   services: [/* ... */],
    -   failErrorConstructor: Error
    - }
    })

    (fyi; failErrorConstructor was implemented from feat: throw cause as error #2 and I think it is very rare use case. I'm considering offering it only as a plugin-level options. not field. cc. @XiNiHa )

  2. When constructing SchemaBuilder, it will be accept runtime that the user wants to use globally. so, plugin doesn't control the runtime initialization and checking it is not the plugin's concern.

    const builder = new SchemaBuilder({
      plugins: [EffectPlugin],
      effectOptions: {
    +   runtime: appRuntime
    +   // runtime: async (context) => appRuntime
      }
    });

    t.effect field resolver will do the following things:

    declare const effectFieldResult: Effect<...>;
    
    return pipe(
      effectFieldResult,
      Runtime.runPromiseExit(effectOptions.runtime)
    ).then((exit) => {
      if (Exit.isFailure(exit)) {
        const cause = Cause.originalError(exit.cause);
    
        if (Cause.isFailType(cause) && cause.error instanceof Error) {
          throw cause.error;
        }
    
        throw new EffectFailureError(Cause.pretty(cause), { cause })
      }
    
      return exit.value;
    })

@vecerek
Copy link
Contributor Author

vecerek commented Oct 25, 2023

@iamchanii I think that looks good as a high-level plan

Regarding the details of:

declare const effectFieldResult: Effect<...>;

return Runtime.runPromise(effectOptions.runtime || Runtime.defaultRuntime)(effectFieldResult);

In the end, I still had to unwrap the error so that the pothos error plugin keeps working like so:

declare const effectFieldResult: Effect<...>;

return pipe(
  effectFieldResult,
  Runtime.runPromiseExit(effectOptions.runtime)
).then((exit) => {
  if (Exit.isFailure(exit)) {
    const cause = Cause.originalError(exit.cause);

    if (Cause.isFailType(cause) && cause.error instanceof Error) {
      throw cause.error;
    }

    throw new Error(Cause.pretty(cause));
  }

  return exit.value;
})

@XiNiHa
Copy link
Contributor

XiNiHa commented Oct 26, 2023

(fyi; failErrorConstructor was implemented from #2 and I think it is very rare use case. I'm considering offering it only as a plugin-level options. not field. cc. @XiNiHa )

Although I agree that the customization ability is very rare to use, throwing the error cause as-is completely breaks the plugin behavior. Therefore I believe the plugin should expose a custom error type (like EffectFailureError?) to throw with a non-error cause.

@iamchanii
Copy link
Owner

(fyi; failErrorConstructor was implemented from #2 and I think it is very rare use case. I'm considering offering it only as a plugin-level options. not field. cc. @XiNiHa )

Although I agree that the customization ability is very rare to use, throwing the error cause as-is completely breaks the plugin behavior. Therefore I believe the plugin should expose a custom error type (like EffectFailureError?) to throw with a non-error cause.

Let's make one assumption: If the plugin user had follow pothos's error plugin introduction then Error interface and BaseError object type should have been defined.

How about just throwing Error which set cause field? if plugin user wants to check original error, could be add new field in BaseError object.

if (Cause.isFailType(cause) && cause.error instanceof Error) {
  throw cause.error;
}

throw new Error(Cause.pretty(cause), { cause });

We can decide to expose a custom error type but, it should be defined as object type somewhere by user or the plugin itself.

@iamchanii
Copy link
Owner

Update 2024: I recently worked on 1.0 of pothos-plugin-effect, and the API has changed in such a way that effectRuntime is now configured in SchemaBuilder. I expect that the initial cause of this issue will be resolved.

Thank you for the many discussions about the API in this issue. I wouldn't have been able to move forward without this discussion. If you get a chance, please take a look at my new work as well.

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

No branches or pull requests

4 participants