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

feat: perform function that does parsing, validation, context assembly and execution/subscription #1515

Open
wants to merge 65 commits into
base: main
Choose a base branch
from

Conversation

enisdenjo
Copy link
Collaborator

@enisdenjo enisdenjo commented Sep 9, 2022

Closes #1493

Necessary for #1490

perform function usage

const { perform } = getEnveloped({ initial: 'ctx' });

const { operationName, query, variables } = JSON.parse(payload);

// Will never throw GraphQL errors
const result = await perform({ operationName, query, variables }, { extension: 'ctx' });

// Result will contain errors for parsing/validation issues or the execution/subscription result(s)
return JSON.stringify(result);

onPerform and onPerformDone plugin usage

import { Plugin } from '@envelop/core';

function usePerform(): Plugin {
  return {
    onPerform({ context, extendContext, params, setParams, setResult }) {
      // before performing

      // context: assembled context
      // extendContext: extend assembled context
      // params: GraphQL parameters passed to perform
      // setParams: replace GraphQL parameters before performing
      // setResult: sets an early result for immediate response

      return {
        onPerformDone({ result, setResult }) {
          // after performing

          // result: perform result, contains errors or result(s)
          // setResult: replace the result
        },
      };
    },
  };
}

TODO

  • Documentation

saihaj and others added 30 commits August 22, 2022 10:48
* experimenting traced schema

* Fix

* remove comment

* do optional chaining since we are not strongly typing

* document

* cleanup

Co-authored-by: Arda TANRIKULU <[email protected]>
* feat: remove enableIf utility

* make types happpy

* make types happy

* add changeset
…#1506)

* remove useAsyncSchema

* rename to useSchemaByContext
…1500)

* on resolve plugin

* changeset

* no more onResolverCalled

* unused import

* args is a record

* integrate and useOnResolve

* resolversHooksSymbol does not exist

* plugincontext for OnPluginInit

* on-resolve uses addPlugin

* onresolvercalled is no more

* refactor for new on-resolve

* fix open-telemetry tests

* fix newrelic

* opentelemetry graphql as peer dep

* tests

* addPlugin doesnt need to be used

* reorder

* respects onPluginInit context

* drop unused import

* fixes false positive eslint warnings

Co-authored-by: Dimitri POSTOLOV <[email protected]>
Copy link
Owner

@n1ru4l n1ru4l left a comment

Choose a reason for hiding this comment

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

There is still no way for applying the onPerformDone logic when not using the perform function. Which makes it impossible for users that do not use perform to leverage plugins that use the onPerformDone hook 🤔 .

I think we should provide such an path as well.

@enisdenjo
Copy link
Collaborator Author

I added the perform function to the testkit and refactored all compatible tests to use it. Things to note:

  • Replaced all usages of execute with perform
  • mockPhase cannot be used with perform; however, no tests use it - does the testkit even need it? Other users?
  • Since perform catches errors and puts them inside the result, the Auth0 plugin test changed a bit: see here.

There is still no way for applying the onPerformDone logic when not using the perform function. Which makes it impossible for users that do not use perform to leverage plugins that use the onPerformDone hook 🤔 .

@n1ru4l that is the thing, if you don't use perform you cannot use onPerform (onPerformDone neither because it's returned from onPerform). Exactly because of this, I put onPerformDone in onPerform instead of it being a top level hook.

Perform is the only function in envelop that wraps parsing and validation errors - you must use it to leverage plugins using these hooks. The problem is described in #1493.

Do you have some alternatives maybe?

@enisdenjo enisdenjo requested a review from n1ru4l September 15, 2022 09:28
saihaj and others added 3 commits September 15, 2022 09:01
* docs: migration guide

* remove slashes

* Update website/docs/guides/migrating-from-v2-to-v3.mdx

Co-authored-by: Denis Badurina <[email protected]>

* feedback

* document removing of orchestrated tracer

* more feedback

* update docs

* update examples

* async schema example

Co-authored-by: Denis Badurina <[email protected]>
@EmrysMyrddin
Copy link
Collaborator

If I understand well what n1ru4l says, it's not a matter of using or not perform.
It's more about plugin developers not being able to use this hook since it can't know in advance if there users will use perform or the original functions. This makes the hook difficult to use and probably not very usefull for most plugins that aims to be reused and distributed as a package.

@enisdenjo
Copy link
Collaborator Author

enisdenjo commented Sep 16, 2022

It's more about plugin developers not being able to use this hook since it can't know in advance if there users will use perform or the original functions. This makes the hook difficult to use and probably not very usefull for most plugins that aims to be reused and distributed as a package.

Yes, I understand this too. But I don't see a lot of feasible solutions, maybe one of them would be: log a warning (throw error? too harsh?) if a plugin uses the perform hooks but the invoker didn't use perform. 🤔

@enisdenjo
Copy link
Collaborator Author

What if we augment each gql function so that it calls onPerformDone so that plugins still get some result manipulation capabilities?

  • parse - catch throws, format as exec result and run through hooks. If the invoker is parse from getEnvloped, take first error from the exec result and bubble it instead
  • validate - caught or returned errors are formatted as exec result and ran through hooks. If invoker is from getEnveloped, return just the errors from the exec result

This way, users can notice when their results are incomplete with respectful plugins and read about how to fix it - the plugins should state this.

However, if the plugin doesn't expect changing full exec results for parsing and validating - nobody will notice.

Still there is this question: how to invoke onPerform? Or alternatively inline onPerformDone?

Base automatically changed from saihaj/engine-agnostic-non-breaking to main October 12, 2022 13:59
@ardatan ardatan force-pushed the main branch 2 times, most recently from a23dec1 to 3ac13be Compare May 23, 2023 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants