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

New API to always wrap execution phase #2153

Open
EmrysMyrddin opened this issue Jan 30, 2024 · 1 comment
Open

New API to always wrap execution phase #2153

EmrysMyrddin opened this issue Jan 30, 2024 · 1 comment

Comments

@EmrysMyrddin
Copy link
Collaborator

EmrysMyrddin commented Jan 30, 2024

The problem

Wrapping the execution of GraphQL operations is a very common need. For example, it is needed for tracing, monitoring, error handling, cache, etc...

Today, this is doable with the setExecuteFn API, which allow to entirely replace the execute function. By keeping a reference to the previous one, it is possible to wrap the execution phase:

const plugin: Plugin = {
  onExecute({ setExecuteFn, executeFn }) {
    setExecuteFn(async (...args) => {
      // The plugin can do things just before the execution begins
      const result = await executeFn(...args)
      // And do things just after the execution has ended.
      return result
    })
  }
}```

But this is fragile, since this API is very permissive and gives a lot of control over the execution phase.
The most problematic side effect is that if a plugin replace the execution function without wrapping the existing one, it will break any subsequent plugin also relying on wrapping the execution functon.

```ts
const wrappingPlugin: Plugin = {
  onExecute({ setExecuteFn, executeFn }) {
    setExecuteFn(async (...args) => {
      console.time('execute')
      const result = await executeFn(...args)
      console.time('execute')
      return result
    })
  }
}

const replacingPlugin: Plugin = {
  onExecute({ setExecuteFn, executeFn }) {
    setExecuteFn(myFancyCustomExecutor)
  }
}

const yoga = createYoga({
  plugins: [
    wrappingPlugin,
    replacingPlugin,
  ],
})

In this example, one would expect both the having a custom executor and the execution time log in the console.
Actually, this is not working, because the replacingPlugin just overrides the wrapped execution function set by wrappingPlugin.
To make it work, the plugin order should be reversed:

const yoga = createYoga({
  plugins: [
	replacingPlugin,
    wrappingPlugin,
  ],
})

The plugins are order dependent, and the order can be know only by understanding the internal implementation details of the plugin, which is really not a good DX, especially for newcomers.

Solution proposal

A solution would be to have a dedicated hook for wrapping the execution phase. This would allow to create some kind of middleware.

To avoid having the same issues than setExecuteFn have, this API should enforce the fact that it can't cutoff the execution chain. All plugins should always be called for every execution. Otherwise, the point of having a new API would be purely a sort of "documentation", relying on the good will of plugin's maintainers to respect the rule.

To enforce this, the responsibility of actually calling the execution function and all the plugin wrapping hooks should remain inside Envelop core.

To achieve that, we could rely on an execution Promise instead of an execution Function. The wrapper will have to await for a promise instead of directly calling the execution function. This way, if the wrapper doesn't await the given Promise, it's a bug in this plugin and it will not break other plugins in the chain.

Here an example of the previous plugin with this new API:

const plugin: Plugin = {
  onExecuteWrap(({ executed }) {
    console.time('execute')
    await executed // We wait for the execution phase to be done, instead of executing it directly
    console.time('executed')
  }
}

It would be possible to have access to potential errors:

const plugin: Plugin = {
  onExecuteWrap(({ executed }) {
    try {
    await executed
    } catch(err) {
      console.error(err)
    }
  }
}

If needed, we can even give access to executionArgs and result:

const plugin: Plugin = {
  onExecuteWrap(({ executionArgs, executed }) {
    const result = await executed
    console.time('executed', executionArgs, result)
  }
}

Perhaps we could even offer a way to set the result, but I'm not sure about this one.

Alternatives

An alternative would be to offer a classic middleware API like this:

const plugin: Plugin = {
  onExecuteMiddleware({ executionArgs, next }) {
    const result = await next(executionArgs)
    return result
  }
}

But the problem with this approach is that nothing prevents the plugin to not call the next function, lead to the exact same problem we currently have with setExecuteFn. The main difference is that since this API is dedicated to wrapping, it would be considered a bug if a plugin breaks the expectation of calling the next function. But compared to the previous proposition, this bug would break the entires plugin chain.

Additional context

This is in the context of multiple users having issues writing reliable tracing or monitoring plugins, such as the OTEL plugin.

@EmrysMyrddin
Copy link
Collaborator Author

To be able to set the execution result, I though of this API, but not sure if it's a good idea to allow this:

const plugin: Plugin = {
  onExecuteWrap(({ executed }) {
    const { result, setResult } = await executed
    setResult(withMetadataExtesion(result))
  }
}

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

1 participant