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: Implementing Conditional Schema Support in @graphql-yoga/nestjs #2992

Merged
merged 11 commits into from
Dec 22, 2023

Conversation

magrinj
Copy link
Contributor

@magrinj magrinj commented Sep 8, 2023

Problem

In the current implementation of graphql-yoga, users have the flexibility to pass a function to the schema property, facilitating the setup of a conditional schema. Unfortunately, this functionality is not mirrored in the @graphql-yoga/nestjs package, creating a limitation when working with frameworks like Nest.JS, which appears to have restricted support for conditional schemas in the GraphQLModule.

At Twenty, we have a specific requirement to determine schemas dynamically based on the tenant to enable custom objects for our customers. The absence of this feature in the @graphql-yoga/nestjs package hinders our ability to provide a more personalized and scalable service.

Solution

To bridge this gap, I have introduced a new option called conditionalSchema. This option allows for the merging of conditional schema directives with the existing schema property, thereby enhancing flexibility and functionality. This enhancement not only preserves the autoSchemaFile option but also introduces the possibility to incorporate conditional schemas within the generated ones.

Changes Made:

  • Introduced a conditionalSchema option in the @graphql-yoga/nestjs package.
  • Ensured compatibility with the existing schema property.
  • Facilitated the integration of conditional schema elements within generated schemas.

Request for Feedback

I'm requiring your feedback on this implementation. Specifically, I am keen to understand potential edge cases or concerns that might arise with this enhancement.

@changeset-bot
Copy link

changeset-bot bot commented Sep 8, 2023

🦋 Changeset detected

Latest commit: 98ff4cd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 24 packages
Name Type
@graphql-yoga/nestjs Major
graphql-yoga Minor
@graphql-yoga/nestjs-federation Patch
@graphql-yoga/render-graphiql Major
@graphql-yoga/plugin-apollo-inline-trace Major
@graphql-yoga/plugin-apq Major
@graphql-yoga/plugin-csrf-prevention Major
@graphql-yoga/plugin-defer-stream Major
@graphql-yoga/plugin-disable-introspection Major
@graphql-yoga/plugin-graphql-sse Major
@graphql-yoga/plugin-jwt Major
@graphql-yoga/plugin-persisted-operations Major
@graphql-yoga/plugin-prometheus Major
@graphql-yoga/plugin-response-cache Major
@graphql-yoga/plugin-sofa Major
apollo-federation-gateway-with-yoga Patch
apollo-subgraph-with-yoga Patch
graphql-lambda Patch
cloudflare-advanced Patch
cloudflare Patch
functions Patch
hackernews Patch
nextjs-app Patch
hello-world-benchmark Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Collaborator

@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.

Can you please add a test that confirms the changes work as expected?

@magrinj magrinj force-pushed the feat/nestjs-conditional-schema branch 2 times, most recently from 75d8320 to 7f7a35c Compare October 5, 2023 08:21
@magrinj
Copy link
Contributor Author

magrinj commented Oct 5, 2023

@n1ru4l I've added a test, let me know if it's of for you !

Copy link
Collaborator

@enisdenjo enisdenjo left a comment

Choose a reason for hiding this comment

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

This looks good, thanks! Can you please rung pnpm changeset to version the package, thanks!

@magrinj magrinj force-pushed the feat/nestjs-conditional-schema branch from fbd52e2 to e0dd0f3 Compare October 11, 2023 07:18
@magrinj
Copy link
Contributor Author

magrinj commented Oct 11, 2023

@enisdenjo I just ran the pnpm changeset command and rebase my PR

Copy link
Collaborator

@enisdenjo enisdenjo left a comment

Choose a reason for hiding this comment

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

Great stuff @magrinj, thank you very much! And sorry for the delay. 😬

Copy link
Collaborator

@enisdenjo enisdenjo left a comment

Choose a reason for hiding this comment

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

Oh, I just saw the CI, I see the build is failing - can you check whats up please?

@magrinj
Copy link
Contributor Author

magrinj commented Oct 12, 2023

@enisdenjo Oh yes sorry, I was using the wrong type for conditionalSchema, instead of creating a new one, I've exposed the one from graphql-yoga package.
I've run pnpm changeset in minor but I'm not sure because both package need to be in their last version to make it work.

.changeset/red-lemons-prove.md Outdated Show resolved Hide resolved
packages/nestjs/src/index.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@enisdenjo enisdenjo left a comment

Choose a reason for hiding this comment

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

Final change. 😄

.changeset/red-lemons-prove.md Outdated Show resolved Hide resolved
@enisdenjo
Copy link
Collaborator

Great job! Thanks!

@enisdenjo
Copy link
Collaborator

@magrinj some tests are failing, can you please check. Sorry for the back and forth...

@magrinj magrinj force-pushed the feat/nestjs-conditional-schema branch from ad32fb9 to 19c74b8 Compare October 13, 2023 06:40
@magrinj
Copy link
Contributor Author

magrinj commented Oct 13, 2023

@enisdenjo Is there any way we can discuss like Discord ? Actually the problem seems a bit complex, and I think I need some suggestion to fix it.
Seems like subscription are broken when I provide a schema property.

@enisdenjo
Copy link
Collaborator

@magrinj I am not very active on Discord, how about Slack? If you're ok with that, please provide me an e-mail and I'll invite you as a guest to our Slack!

@magrinj
Copy link
Contributor Author

magrinj commented Oct 14, 2023

@enisdenjo Of course no I've Slack too, you can invite me on contact@magrin.fr
Thanks !

@enisdenjo
Copy link
Collaborator

@magrinj invite's out!

@magrinj
Copy link
Contributor Author

magrinj commented Oct 16, 2023

@enisdenjo Thanks a lot, I'll try to reach you tomorrow if you're available

@enisdenjo
Copy link
Collaborator

Great! Looking forward to hearing from you.

@magrinj magrinj force-pushed the feat/nestjs-conditional-schema branch from 2338efa to 4ee697e Compare October 17, 2023 14:51
@magrinj
Copy link
Contributor Author

magrinj commented Oct 19, 2023

@enisdenjo Ok so for now, I've disabled conditionalSchema option with subscriptions.
I'm trying to make another PR with your help to implement the subscription support.

@enisdenjo
Copy link
Collaborator

Great job @magrinj! I'll merge this and cut a new version.

@enisdenjo
Copy link
Collaborator

@magrinj now we have Bun checks failing (see job). 😬

Can you check it out?

@magrinj
Copy link
Contributor Author

magrinj commented Oct 26, 2023

@enisdenjo Hello, sorry for the delay, I've some days off this week.
I don't really understand what's happening for now, I don't think I'll have time to take a look this week, but maybe next week !

@magrinj magrinj force-pushed the feat/nestjs-conditional-schema branch from f775b5a to 98ff4cd Compare December 18, 2023 11:40
@magrinj
Copy link
Contributor Author

magrinj commented Dec 18, 2023

@enisdenjo I've fix the issue, it was a small one sorry of the delay...

Copy link
Collaborator

@enisdenjo enisdenjo left a comment

Choose a reason for hiding this comment

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

@magrinj this looks great to me, we ready to merge?

@EmrysMyrddin
Copy link
Collaborator

@enisdenjo Did you noticed that the changeset set bot is triggering a major version on lots of packaging ? Not sure why

@enisdenjo
Copy link
Collaborator

Good catch. 🤔

I don't know why though. The changesets in this PR are all minor. Could be that the bot has a brainfart?

@EmrysMyrddin
Copy link
Collaborator

I don't know ^^'
Do you think we can just merge it and see what happens on master ?

@enisdenjo
Copy link
Collaborator

Do you think we can just merge it and see what happens on master ?

Let's see. 😄

@enisdenjo enisdenjo merged commit b1f0e3a into dotansimha:main Dec 22, 2023
25 checks passed
Copy link
Contributor

✅ Benchmark Results

     ✓ no_errors{mode:graphql}
     ✓ expected_result{mode:graphql}
     ✓ no_errors{mode:graphql-jit}
     ✓ expected_result{mode:graphql-jit}
     ✓ no_errors{mode:graphql-response-cache}
     ✓ expected_result{mode:graphql-response-cache}
     ✓ no_errors{mode:graphql-no-parse-validate-cache}
     ✓ expected_result{mode:graphql-no-parse-validate-cache}

     checks.......................................: 100.00% ✓ 422316      ✗ 0     
     data_received................................: 1.7 GB  14 MB/s
     data_sent....................................: 85 MB   710 kB/s
     http_req_blocked.............................: avg=1.39µs   min=941ns    med=1.27µs   max=245.28µs p(90)=1.78µs   p(95)=1.99µs  
     http_req_connecting..........................: avg=2ns      min=0s       med=0s       max=130.48µs p(90)=0s       p(95)=0s      
     http_req_duration............................: avg=363.6µs  min=199.5µs  med=319.29µs max=14.1ms   p(90)=519.47µs p(95)=538.74µs
       { expected_response:true }.................: avg=363.6µs  min=199.5µs  med=319.29µs max=14.1ms   p(90)=519.47µs p(95)=538.74µs
     ✓ { mode:graphql-jit }.......................: avg=274.4µs  min=199.5µs  med=254.21µs max=14.1ms   p(90)=284.84µs p(95)=295.85µs
     ✓ { mode:graphql-no-parse-validate-cache }...: avg=546.16µs min=432.38µs med=519.54µs max=9.79ms   p(90)=558.68µs p(95)=587.76µs
     ✓ { mode:graphql-response-cache }............: avg=337.85µs min=264.44µs med=319.97µs max=6.6ms    p(90)=351.51µs p(95)=361.79µs
     ✓ { mode:graphql }...........................: avg=359.61µs min=264.17µs med=328.11µs max=12.75ms  p(90)=371.7µs  p(95)=413.52µs
     http_req_failed..............................: 0.00%   ✓ 0           ✗ 211158
     http_req_receiving...........................: avg=31.78µs  min=16.22µs  med=31.88µs  max=7.19ms   p(90)=37.23µs  p(95)=39.36µs 
     http_req_sending.............................: avg=7.67µs   min=5.38µs   med=6.98µs   max=379.04µs p(90)=10.16µs  p(95)=10.86µs 
     http_req_tls_handshaking.....................: avg=0s       min=0s       med=0s       max=0s       p(90)=0s       p(95)=0s      
     http_req_waiting.............................: avg=324.13µs min=168.75µs med=280.62µs max=14.02ms  p(90)=479.92µs p(95)=497.27µs
     http_reqs....................................: 211158  1759.627026/s
     iteration_duration...........................: avg=563.66µs min=365.37µs med=515.54µs max=15.96ms  p(90)=722.9µs  p(95)=746.31µs
     iterations...................................: 211158  1759.627026/s
     vus..........................................: 1       min=1         max=1   
     vus_max......................................: 2       min=2         max=2   

@enisdenjo
Copy link
Collaborator

@EmrysMyrddin seems like the changesets are a-ok. See #3148.

@magrinj
Copy link
Contributor Author

magrinj commented Dec 22, 2023

Thanks for the merge ! 🙂

This was referenced May 7, 2024
This was referenced May 23, 2024
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

Successfully merging this pull request may close these issues.

4 participants