feat: added onExtendSchema application hook

stearm commented 1 year ago

Added onExtendSchema hook, as requested in https://github.com/mercurius-js/validation/issues/39.

An issue with the extendSchema function is that, because the hook handler is called inside the function and returns a promise, waiting for the promise will cause extendSchema to become async as well, which would be a breaking change. Does this make sense?

simoneb commented 1 year ago

@stearm thanks for this PR. can you take a look why CI is red?

stearm commented 1 year ago

@stearm thanks for this PR. can you take a look why CI is red?

The reason is that now there isn't a 100% coverage, because of this update.

I'm trying to test the left part of the || but actually, it seems to be never called. Before my update it was:

fastifyGraphQl.extendSchema = fastifyGraphQl.extendSchema || function (s) { ... }

But since the graphql field is created by mercurius itself, I don't think there is a chance that extendSchema is already defined.

For example mercurius-gateway is replacing the extendSchema function here, but in this way it overrides the entire implementation of mercurius, isn't it?

If this is true, the implementation becomes:

fastifyGraphQl.extendSchema = (s) => {
    if (typeof s === 'string') {
      s = parse(s)
    } else if (!s || typeof s !== 'object') {
      throw new MER_ERR_INVALID_OPTS('Must provide valid Document AST')
    }

    fastifyGraphQl.schema = extendSchema(fastifyGraphQl.schema, s)

    const context = assignApplicationHooksToContext({}, fastifyGraphQl[kHooks])

    if (context.onExtendSchema !== null) {
      onExtendSchemaHandler({ schema: fastifyGraphQl.schema, context })
    }
  }

and so the coverage will become green again. Wdyt @simoneb ?

simoneb commented 1 year ago

Thanks for this PR @stearm. A few comments:

I think we need to preserve the existing behavior that, if extendSchema already exists, we don't re-define it. It's unlikely to happen as you said, but there's no reason to remove this behavior either
the gateway package definitely overrides this method because apparently it's doing something with the schema which doesn't allow calling extendSchema after you register the gateway, which I guess is fine and we shouldn't do anything about it
about the asynchronous nature of this hook, good point. I suspect that this will require making extendSchema async, which would be breaking change. Before we go that far, I think it would be worth checking something that may have been done in the past around this. More specifically, this looks like the first application lifecycle hook (as opposed to a request lifecycle hook) that we're implementing. In the docs about hooks, I can see at the bottom of the page a reference to a similar hook that apparently doesn't exist anymore now (maybe the docs weren't updated properly): https://mercurius.dev/#/docs/hooks?id=graphql-application-lifecycle-hooks. Check if you can find some info about this to get a sense of how it was implemented before, or perhaps if it's a hook that was moved to the gateway instead

simoneb commented 1 year ago

@mcollina some guidance here might be useful, especially on the last point of my comment above about the possible async nature of this hook.

mcollina commented 1 year ago

I don’t see a problem in the function becoming asynchronous (minus the wasted promises). That's not a hot path and it would reduce the complexity of the solution.

mcollina commented 1 year ago

(CI is failing)

simoneb commented 1 year ago

I don’t see a problem in the function becoming asynchronous (minus the wasted promises). That's not a hot path and it would reduce the complexity of the solution.

It would be a breaking change though, as anybody calling extendSchema would now get back a promise to await

simoneb commented 1 year ago

(CI is failing)

yep we know, broken while waiting for some direction where to go, as it fails due to missing test coverage

mcollina commented 1 year ago

Ah now I understand. No, I don't think we should make extendSchema async.

simoneb commented 1 year ago

@stearm given Matteo's feedback above, I'd check if we're missing anything as I mentioned in the this point of my earlier comment. Maybe there's a way (or actually, a compromise of some sort) that doesn't require making extendSchema async. Let's keep digging for a little longer.

stearm commented 1 year ago

I see the onGatewayReplaceSchemaHandler on mercurius-gateway (that is an application hook) is also async. I'm unable to see a non-disruptive way to make the onExtendSchema hook syncronous, and then the extendSchema. Maybe a breaking change is needed here, what do you think? @simoneb @mcollina

simoneb commented 1 year ago

I see the onGatewayReplaceSchemaHandler on mercurius-gateway (that is an application hook) is also async. I'm unable to see a non-disruptive way to make the onExtendSchema hook syncronous, and then the extendSchema. Maybe a breaking change is needed here, what do you think? @simoneb @mcollina

I agree that there's no obvious non-breaking way to introduce this change. Since this pattern already exists in the gateway I think it's reasonable to introduce it here as well, but clearly we'd have to wait until the next semver major.

stearm commented 1 year ago

I see the onGatewayReplaceSchemaHandler on mercurius-gateway (that is an application hook) is also async. I'm unable to see a non-disruptive way to make the onExtendSchema hook syncronous, and then the extendSchema. Maybe a breaking change is needed here, what do you think? @simoneb @mcollina

I agree that there's no obvious non-breaking way to introduce this change. Since this pattern already exists in the gateway I think it's reasonable to introduce it here as well, but clearly we'd have to wait until the next semver major.

✅ ~~If the implementation is ok, I can proceed and update the docs.~~

mcollina commented 1 year ago

Seems to keep failing on Node v20.

simoneb commented 1 year ago

Seems to keep failing on Node v20.

We noticed, but it doesn't look like the issue is being caused by changes in this PR. The build on Node 20 has failed in the past also on master, and also because of timeouts. I've had a quick look but I could pinpoint the issue to any specific changes unfortunately, and it's probably unlikely to be caused by recent changes either, but rather Node 20 itself, and for reasons I couldn't figure out.

mcollina commented 1 year ago

Seems to keep failing on Node v20.

We noticed, but it doesn't look like the issue is being caused by changes in this PR. The build on Node 20 has failed in the past also on master, and also because of timeouts. I've had a quick look but I could pinpoint the issue to any specific changes unfortunately, and it's probably unlikely to be caused by recent changes either, but rather Node 20 itself, and for reasons I couldn't figure out.

The last commit on master passed in https://github.com/mercurius-js/mercurius/actions/runs/4850505716/jobs/8643465117.

simoneb commented 1 year ago

The last commit on master passed in https://github.com/mercurius-js/mercurius/actions/runs/4850505716/jobs/8643465117.

I know, but two before failed for timeout reasons too https://github.com/mercurius-js/mercurius/actions/runs/4818964292/jobs/8581598028

stearm commented 1 year ago

The last commit on master passed in https://github.com/mercurius-js/mercurius/actions/runs/4850505716/jobs/8643465117.

I know, but two before failed for timeout reasons too https://github.com/mercurius-js/mercurius/actions/runs/4818964292/jobs/8581598028

I add for info that tests are running fine on my machine using node v20 (macOS).

mcollina commented 1 year ago

Please rebase/merge on master because it fixes the Node v20 issue

stearm commented 1 year ago

Please rebase/merge on master because it fixes the Node v20 issue

✅

mcollina commented 1 year ago

@simoneb are you ok with this or are there further changes?

simoneb commented 1 year ago

LGTM

mercurius-js / mercurius

feat: added onExtendSchema application hook #985