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

[jwt] fix typo in plugin options name #3426

Merged
merged 5 commits into from
Nov 5, 2024
Merged

Conversation

EmrysMyrddin
Copy link
Collaborator

No description provided.

Copy link

changeset-bot bot commented Sep 3, 2024

🦋 Changeset detected

Latest commit: d737143

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

This PR includes changesets to release 1 package
Name Type
@graphql-yoga/plugin-jwt 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

@EmrysMyrddin EmrysMyrddin force-pushed the jwt/fix/option_name_typo branch from 27b8a88 to 9238163 Compare September 3, 2024 12:27
Copy link
Contributor

github-actions bot commented Sep 3, 2024

✅ 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}
     ✓ no_errors{mode:uws}
     ✓ expected_result{mode:uws}

     checks.......................................: 100.00% ✓ 517828      ✗ 0     
     data_received................................: 2.1 GB  14 MB/s
     data_sent....................................: 104 MB  694 kB/s
     http_req_blocked.............................: avg=1.5µs    min=982ns    med=1.33µs   max=293.87µs p(90)=1.98µs   p(95)=2.17µs  
     http_req_connecting..........................: avg=2ns      min=0s       med=0s       max=160.86µs p(90)=0s       p(95)=0s      
     http_req_duration............................: avg=363.65µs min=219.63µs med=327.03µs max=18.28ms  p(90)=462.36µs p(95)=484.55µs
       { expected_response:true }.................: avg=363.65µs min=219.63µs med=327.03µs max=18.28ms  p(90)=462.36µs p(95)=484.55µs
     ✓ { mode:graphql-jit }.......................: avg=293.46µs min=219.63µs med=270.98µs max=18.28ms  p(90)=303.02µs p(95)=319.44µs
     ✓ { mode:graphql-no-parse-validate-cache }...: avg=492.38µs min=399.98µs med=463.81µs max=9.65ms   p(90)=507.27µs p(95)=536.52µs
     ✓ { mode:graphql-response-cache }............: avg=345.47µs min=267.8µs  med=326.02µs max=8.87ms   p(90)=358.13µs p(95)=371.88µs
     ✓ { mode:graphql }...........................: avg=371.06µs min=273.45µs med=334.67µs max=13.31ms  p(90)=392.45µs p(95)=441.12µs
     ✓ { mode:uws }...............................: avg=350.89µs min=271.51µs med=327.61µs max=9.58ms   p(90)=363.32µs p(95)=383.45µs
     http_req_failed..............................: 0.00%   ✓ 0           ✗ 258914
     http_req_receiving...........................: avg=33.5µs   min=16.52µs  med=33.32µs  max=831.23µs p(90)=39.49µs  p(95)=42.18µs 
     http_req_sending.............................: avg=8.6µs    min=6.03µs   med=7.63µs   max=305.22µs p(90)=11.07µs  p(95)=12.03µs 
     http_req_tls_handshaking.....................: avg=0s       min=0s       med=0s       max=0s       p(90)=0s       p(95)=0s      
     http_req_waiting.............................: avg=321.54µs min=183.87µs med=285.78µs max=18.15ms  p(90)=419.56µs p(95)=439.63µs
     http_reqs....................................: 258914  1726.074272/s
     iteration_duration...........................: avg=574.42µs min=384.61µs med=535µs    max=18.92ms  p(90)=677.58µs p(95)=704.95µs
     iterations...................................: 258914  1726.074272/s
     vus..........................................: 1       min=1         max=1   
     vus_max......................................: 2       min=2         max=2   

Copy link
Contributor

github-actions bot commented Sep 3, 2024

Apollo Federation Subgraph Compatibility Results

Federation 1 Support Federation 2 Support
_service🟢
@key (single)🟢
@key (multi)🟢
@key (composite)🟢
repeatable @key🟢
@requires🟢
@provides🟢
federated tracing🟢
@link🟢
@shareable🟢
@tag🟢
@override🟢
@inaccessible🟢
@composeDirective🟢
@interfaceObject🟢

Learn more:

@EmrysMyrddin EmrysMyrddin force-pushed the jwt/fix/option_name_typo branch from add8733 to d737143 Compare November 4, 2024 14:08
Copy link
Contributor

github-actions bot commented Nov 4, 2024

💻 Website Preview

The latest changes are available as preview in: https://b2256dac.graphql-yoga.pages.dev

@@ -539,7 +539,7 @@ describe('jwt plugin', () => {
const secret = 'topsecret';
const test = createTestServer(
{
singingKeyProviders: [createInlineSigningKeyProvider(secret)],
signingKeyProviders: [createInlineSigningKeyProvider(secret)],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a test for the backward compatibility to prevent future regressions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean you want the old misspelled option should continue to work and we should have a test for this ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, Until we completely remove it, let's make sure it works.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh i just realized this change would break the existing users because it needs signing one as required while the old one is optional now. A union type would be better no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Aaaah you're right ! I will fix this and add test

@EmrysMyrddin EmrysMyrddin requested a review from ardatan November 5, 2024 14:00
@EmrysMyrddin EmrysMyrddin merged commit 076d25c into main Nov 5, 2024
35 checks passed
@EmrysMyrddin EmrysMyrddin deleted the jwt/fix/option_name_typo branch November 5, 2024 15:01
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.

2 participants