responseCache plugin additional extension metadata when disabled

[mmadson]

Is your feature request related to a problem? Please describe.

When enabled expression evaluates to false and includeExtensionMetadata is set to true, there is no indication in the response that caching did or did not occur.

Describe the solution you'd like

Given

• enabled expression is supplied to the responseCache plugin configuration
• includeExtensionMetadata is set to true for the responseCache plugin configuration

When

• enabled expression evaluates to false

Then

Include extension metadata indicating that

• hit: false
• didCache: false
• enabled: false

This new enabled field in the extension metadata should also be present for other responses when enabled evaluates to true and should indicate true in these cases.

Describe alternatives you've considered

Patching the lib via package-patch

Additional context

N/A

[mmadson]

@n1ru4l thoughts on this? I have a local patch that adds the enabled field to the extension metadata, but I'm not sure it's implemented in an elegant way. Happy to work with you on this if you think it's a valuable feature though.

[EmrysMyrddin]

Hi @mmadson ! Sorry for the response delay here.
This seems a reasonable feature to add :-) PR are very welcome if you want to implement this!

If you're not sure how to do it, don't hesitate to create a draft PR and ask any questions :-) We are here to help!

[mmadson]

Thanks @EmrysMyrddin, no worries about the delay. I'll take a pass at an implementation and get back to ya asap.

[mmadson]

Okay so here is the issue I'm having @EmrysMyrddin, hopefully you have a suggestion.

I'd like enabled: false to appear whenever includeExtensionMetadata is true but response cache is disabled. And I'd like enabled: true to appear whenever includeExtensionMetadata is true and response cache is enabled.

For the enabled: true case, it's pretty simple. I just need to add enabled: true to all the current resultWithMetadata calls in the envelope response cache plugin:

• https://github.com/n1ru4l/envelop/blob/main/packages/plugins/response-cache/src/plugin.ts#L533
• https://github.com/n1ru4l/envelop/blob/main/packages/plugins/response-cache/src/plugin.ts#L548
• https://github.com/n1ru4l/envelop/blob/main/packages/plugins/response-cache/src/plugin.ts#L555

The enabled: false case is where things get tricky.

Instead of simply returning on this line

• https://github.com/n1ru4l/envelop/blob/main/packages/plugins/response-cache/src/plugin.ts#L388

I need to add the enabled: false to the result metadata: so I was thinking something like:

return {
    onExecuteDone({ result, setResult }) {
        if (isAsyncIterable(result)) {
            return handleAsyncIterableResult(disabledResult);
        }
        return disabledResult(result, setResult);
    },
};

where disabledResult returns something like:

function disabledResult(result, setResult) {
    if (includeExtensionMetadata) {
        setResult(resultWithMetadata(result, { hit: false, didCache: false, enabled: false }));
    }
} 

This gets us most of the way there, but there is still one bit of metadata related to the response cache plugin that for some reason gets set from the graphql yoga package. I believe this is the case when there is a cache hit.

So we'd need to modify this line to include enabled: true

• https://github.com/dotansimha/graphql-yoga/blob/main/packages/plugins/response-cache/src/index.ts#L180

The fact that this metadata is spread across multiple projects is a bit surprising so let me know if I've missed something. If you agree with all of this, I can go ahead and get the 2 PRs ready -- one for this project and one for graphql yoga. Not really sure how to coordinate releases.

[ardatan]

I am not sure if we really need an extension metadata when it is disabled.
Because when it is enabled, we always have the extension metadata so it means it is enabled.
If the metadata doesn't exist, it means it is disabled.

[n1ru4l]

Unless there is another reason on why we dould need this new field, I agree with @ardatan. Afaik dows not provide any additional value over checking whether the extendion field exists.

[mmadson]

@ardatan honestly, I didn't even think about using the presence of the root metadata node to detect if caching was or wasn't enabled -- so now that you mention it, I suppose you are right, that would be sufficient. Thanks for taking the time to consider this issue, feel free to consider it resolved.