feat(types): allow plugin methods to return synchronously#8208
Conversation
|
@vishwakt: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
✅ AI Style Review — No Changes DetectedNo MDX files were changed in this pull request. Review Log: View detailed log
|
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📜 Recent review details🔇 Additional comments (3)
📝 WalkthroughWalkthroughThis PR introduces a new exported PromiseOrValue and widens plugin and listener hook types to accept synchronous or Promise returns, updates hook-invocation utilities to handle both forms, and adds a test verifying synchronous plugin hook behavior. ChangesSynchronous plugin hook support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Microsoft Presidio Analyzer (2.2.362).changeset/sharp-animals-talk.mdMicrosoft Presidio Analyzer failed to scan this file Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/src/utils/invokeHooks.ts (1)
17-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAsync end-hook Promises are silently dropped —
didEndHookresult must be awaited.The returned wrapper is declared
async, and callersawaitit (so they expect all end-hook work to have completed). However, the innerdidEndHook(...args)call discards its return value. If any collected end hook returns aPromise<void>(now expressible viaPromiseOrValue<void>), that promise is silently dropped and the server proceeds before the async work finishes.This was a pre-existing gap, but this PR widens the type to
PromiseOrValue<void>precisely to support async returning hooks, making it more likely that real async end hooks will be silently ignored at runtime.🐛 Proposed fix — await each end hook in series
return async (...args: TEndHookArgs) => { for (const didEndHook of didEndHooks) { - didEndHook(...args); + await didEndHook(...args); } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/utils/invokeHooks.ts` around lines 17 - 21, The returned async wrapper currently invokes each didEndHook without awaiting their results, which drops Promises; update the returned function inside invokeHooks (the async (...args: TEndHookArgs) => { ... }) to await each didEndHook(...args) so asynchronous end-hooks complete before returning (i.e., replace didEndHook(...args) with await didEndHook(...args) while iterating over didEndHooks to run them in series).
🧹 Nitpick comments (1)
packages/server/src/utils/invokeHooks.ts (1)
4-4: 💤 Low value
AsyncDidEndHookis a misnomer now that the return type isPromiseOrValue<void>.Since the type union includes plain
void, calling it "Async" is misleading. Consider renaming toMaybeAsyncDidEndHookor justDidEndHookto match the widened semantics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/utils/invokeHooks.ts` at line 4, The type AsyncDidEndHook<TArgs extends any[]> is misleading because its return is PromiseOrValue<void>, so rename the type to DidEndHook<TArgs extends any[]> (or MaybeAsyncDidEndHook) and update all references/usages in this module (e.g., functions or arrays that accept AsyncDidEndHook in invokeHooks.ts) to the new name; adjust any exported type names and import sites accordingly to keep public API consistent and run a quick compile to fix any remaining identifier references.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/server/src/__tests__/ApolloServer.test.ts`:
- Around line 822-853: The test uses a single syncCalled flag so serverWillStart
immediately makes the final assertion useless; change syncPlugin to track
separate flags (e.g., startedCalled, stoppedCalled, willSendResponseCalled) or a
counter for each hook inside serverWillStart, serverWillStop, requestDidStart,
and willSendResponse, then update the final assertions to verify each specific
flag/counter was incremented (or true) to ensure all three synchronous hooks
(serverWillStart, serverWillStop, requestDidStart->willSendResponse) actually
ran.
In `@packages/server/src/externalTypes/plugins.ts`:
- Line 28: The public export list in externalTypes/index.ts is missing the
PromiseOrValue type (defined in plugins.ts) so plugin authors can't import it;
update the export block that re-exports from './plugins.js' to include
PromiseOrValue alongside ApolloServerPlugin, GraphQLFieldResolverParams, etc.,
so PromiseOrValue becomes part of the package's public API and can be imported
from `@apollo/server` by plugin authors.
---
Outside diff comments:
In `@packages/server/src/utils/invokeHooks.ts`:
- Around line 17-21: The returned async wrapper currently invokes each
didEndHook without awaiting their results, which drops Promises; update the
returned function inside invokeHooks (the async (...args: TEndHookArgs) => { ...
}) to await each didEndHook(...args) so asynchronous end-hooks complete before
returning (i.e., replace didEndHook(...args) with await didEndHook(...args)
while iterating over didEndHooks to run them in series).
---
Nitpick comments:
In `@packages/server/src/utils/invokeHooks.ts`:
- Line 4: The type AsyncDidEndHook<TArgs extends any[]> is misleading because
its return is PromiseOrValue<void>, so rename the type to DidEndHook<TArgs
extends any[]> (or MaybeAsyncDidEndHook) and update all references/usages in
this module (e.g., functions or arrays that accept AsyncDidEndHook in
invokeHooks.ts) to the new name; adjust any exported type names and import sites
accordingly to keep public API consistent and run a quick compile to fix any
remaining identifier references.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c4b0c9d-2c30-4692-8842-499bdd5b4919
📒 Files selected for processing (3)
packages/server/src/__tests__/ApolloServer.test.tspackages/server/src/externalTypes/plugins.tspackages/server/src/utils/invokeHooks.ts
📜 Review details
🔇 Additional comments (1)
packages/server/src/externalTypes/plugins.ts (1)
44-190: LGTM — all hook signatures consistently widened.The
PromiseOrValuesubstitution is uniform acrossApolloServerPlugin,GraphQLServerListener, andGraphQLRequestListener. Intentionally excluded hooks (schemaDidLoadOrUpdate,willResolveField) are correctly left untouched.
vishwakt
force-pushed
the
fix-plugin-method-types-issue-8146
branch
from
df7b1ea to
34d8401
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.changeset/sharp-animals-talk.md (1)
1-3:⚠️ Potential issue | 🟠 Major | ⚡ Quick winChangeset file is empty and will not trigger a release.
The changeset file contains only the YAML delimiters but is missing the package name, version bump type, and release notes. This will prevent the changes from being included in the next release.
📝 Proposed changeset content
--- +"@apollo/server": patch --- - + +Plugin and listener method types now accept synchronous returns via `PromiseOrValue<T>`, eliminating the need to mark simple plugin hooks `async` when no asynchronous work is performed.Note: Consider using
minorinstead ofpatchif this is treated as a new feature (exportingPromiseOrValue<T>type). Usepatchif it's purely a type fix/improvement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.changeset/sharp-animals-talk.md around lines 1 - 3, The changeset file is empty so it won't trigger a release; open .changeset/sharp-animals-talk.md and replace the empty YAML with a proper changeset entry including the package name(s) (e.g., the package that exports PromiseOrValue<T>), the bump type (patch or minor as appropriate), and a short release note describing the change (e.g., "export PromiseOrValue<T> type"); ensure the top-level YAML keys follow changeset format (packages: or <package-name>: <bump>) and include the summary under the body so the changeset will be picked up by the release tooling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.changeset/sharp-animals-talk.md:
- Around line 1-3: The changeset file is empty so it won't trigger a release;
open .changeset/sharp-animals-talk.md and replace the empty YAML with a proper
changeset entry including the package name(s) (e.g., the package that exports
PromiseOrValue<T>), the bump type (patch or minor as appropriate), and a short
release note describing the change (e.g., "export PromiseOrValue<T> type");
ensure the top-level YAML keys follow changeset format (packages: or
<package-name>: <bump>) and include the summary under the body so the changeset
will be picked up by the release tooling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7bd2f787-6d2c-4bbf-a399-108ed5fbb4be
📒 Files selected for processing (4)
.changeset/sharp-animals-talk.mdpackages/server/src/__tests__/ApolloServer.test.tspackages/server/src/externalTypes/plugins.tspackages/server/src/utils/invokeHooks.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/server/src/tests/ApolloServer.test.ts
- packages/server/src/externalTypes/plugins.ts
📜 Review details
🔇 Additional comments (2)
packages/server/src/utils/invokeHooks.ts (2)
2-14: Typing update is aligned and backward compatible.Allowing
PromiseOrValuehere cleanly supports sync hook implementations without breaking existing async plugin hooks.
47-50: Good widening of hook return contract.This change correctly permits synchronous values while preserving existing control flow and return semantics.
Closes apollographql#8146 This updates the ApolloServerPlugin and various listener method types to return PromiseOrValue<T> instead of Promise<T>. This allows plugin authors to write synchronous methods without needing to use async/await when they don't actually perform asynchronous work. Internal utilities like invokeHooks were also updated to support and correctly type the mixed asynchronous returns.
vishwakt
force-pushed
the
fix-plugin-method-types-issue-8146
branch
from
34d8401 to
69128d3
Compare
2 participants
Fixes #8146
Description
This PR updates the
ApolloServerPluginand various listener method types to allow synchronous returns by introducing and using aPromiseOrValue<T>helper instead of strictly requiringPromise<T>.Previously, plugin authors were forced to define their methods as
asyncor explicitly return a resolvedPromise, even when their implementation performed no asynchronous operations (e.g., simple logging or stat collection). This created friction, especially when porting existing JavaScript plugins to TypeScript.This change strictly widens the types, making it purely backward compatible. All internal utilities, such as the
invokeHooksmethods, already await or cleanly handle non-promise values seamlessly throughPromise.alland standard asynchronous loops, so the runtime behavior remains completely unaffected.Changes Made
export type PromiseOrValue<T> = Promise<T> | T;inpackages/server/src/externalTypes/plugins.ts.Promise<T>withPromiseOrValue<T>on plugin hooks (requestDidStart,serverWillStart, etc.) and event listener methods.invokeHooks.tsinternals to correctly type the hooked functions usingPromiseOrValue.packages/server/src/__tests__/ApolloServer.test.tsto ensure that fully synchronous plugin methods compile and execute correctly.Testing
npm run compileto verify there are no TypeScript errors.npm run testand confirmed all tests pass locally.Summary by CodeRabbit
New Features
Tests
Documentation