fix: Extract request object from correct execution context#1403
fix: Extract request object from correct execution context#1403benkenawell wants to merge 2 commits into
Conversation
benkenawell
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces optional GraphQL support to the NestJS package. The primary changes involve adding @nestjs/graphql as an optional peer dependency and implementing a new utility function, getRequestFromContext, which allows decorators to retrieve the request object from both HTTP and GraphQL execution contexts. Feedback was provided regarding a potential runtime error in this utility function; specifically, the implementation needs more robust optional chaining to safely handle environments where the optional GraphQL dependency is not installed.
benkenawell
force-pushed
the
bugfix/req-flags-enabled
branch
from
cf4455e to
b3916a2
Compare
Unfortunately, if using the decorator on a graphql resolver or operation, the request object lives in a slightly different place. This commit adds @nestjs/graphql as an optional peer dependency and adds support in the RequireFlagsEnabled decorator for the graphql context. Signed-off-by: Benjamin Kenawell <ben@withremark.com>
benkenawell
force-pushed
the
bugfix/req-flags-enabled
branch
from
b3916a2 to
79bccc5
Compare
| import type { Client, EvaluationContext } from '@openfeature/server-sdk'; | ||
| import { OpenFeature } from '@openfeature/server-sdk'; | ||
| import { ExecutionContext } from '@nestjs/common'; | ||
| import type { GqlExecutionContext as GqlExecutionContextClass, GqlContextType } from '@nestjs/graphql'; |
There was a problem hiding this comment.
I know this is just importing the types but could this be an issue if @nestjs/graphql isn't available?
There was a problem hiding this comment.
I created this branch as a little sample repository for showing that this shouldn't be an issue if @nestjs/graphql isn't available. I have my changes vendored in, so you could clone and install and try it if you want. (I realize that sounds pretty insecure. Changing the install path for those packages to this PR would also work. I only vendored them in to make it easier to share)
beeme1mr
left a comment
beeme1mr
left a comment
There was a problem hiding this comment.
Thanks for the PR @benkenawell. It looks good from what I can tell. Would it be possible to add a test?
|
yes! It might be a few days before I get the chance, but I would be happy to write a few tests. I believe the types won't be an issue, but I will double check that by importing into a new nest project as well! |
Signed-off-by: Benjamin Kenawell <ben@withremark.com>
benkenawell
force-pushed
the
bugfix/req-flags-enabled
branch
from
c72f571 to
903a553
Compare
… of [my fork of open-feature/js-sdk](https://github.com/benkenawell/openfeature-js-sdk/tree/bugfix/req-flags-enabled) for [this PR](open-feature/js-sdk#1403) Shows that you can build and run without @nestjs/graphql
beeme1mr
left a comment
beeme1mr
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks @benkenawell
@lukas-reining would you mind taking a look when you have a moment?
|
Sorry for the delay, I am on a conference right now and will try to make sure to look latest next week @beeme1mr @benkenawell! |
| let GqlExecutionContext: typeof GqlExecutionContextClass | undefined; | ||
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-var-requires, @typescript-eslint/consistent-type-imports | ||
| ({ GqlExecutionContext } = require('@nestjs/graphql') as typeof import('@nestjs/graphql')); |
There was a problem hiding this comment.
I think this won't ESM build; we'll get "Dynamic require not supported" in true ESM and the try/catch silently swallows it. Could you switch to await import('@nestjs/graphql')? intercept() is already async so it should be pretty straightforward unless I'm missing something.
4 participants
This PR
Adds graphql context support to the RequireFlagsEnabled decorator in the nestjs package.
Unfortunately, if using the decorator on a graphql resolver or operation, the request object lives in a slightly different place. This PR adds @nestjs/graphql as an optional peer dependency and adds support in the RequireFlagsEnabled decorator for the graphql context. If you aren't using nestjs/graphql, this should still work fine. If you are, it should also transparently work.
Related Issues
Fixes #1369
Notes
Follow-up Tasks
There may be other places where the request object is accessed, those should also use the
getRequestFromContexthelper function I've added to theutils.tsfile.How to test
npm ci and npm test should cover it.