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

Tolerate undefined concast.sources if complete called earlier than concast.start #10526

Merged
merged 2 commits into from
Feb 6, 2023

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Feb 6, 2023

Fixes issue #10262, since this.sources.shift() throws when complete is called called before the Promise that should have initialized this.sources resolves, which can happen when the directive combination @client @export(as: ...) is used, because @export usage causes a Promise<Observable[]> to be passed to the Concast constructor instead of passing the Observable[] itself.

Although passing a Promise to the Concast constructor should work, we only exercise that code path in one place, and the additional wrinkle of calling complete before the Promise resolves was never tested.

@benjamn benjamn self-assigned this Feb 6, 2023
@changeset-bot

This comment was marked as resolved.

Fixes issue #10262, since `this.sources.shift()` throws when `complete`
is called called before the `Promise` that should have initialized
`this.sources` resolves, which can happen when the directive combination
`@client @export(as: ...)` is used, because `@export` usage causes a
`Promise<Observable[]>` to be passed to the `Concast` constructor
instead of the `Observable[]` itself.

Although passing a `Promise` to the `Concast` constructor should work,
we only exercise that code path in one place, and the additional wrinkle
of calling `complete` before the `Promise` resolves was never tested:
https://github.com/apollographql/apollo-client/blob/015a1ffff3febe4604956f4bb137a3111f3d8257/src/core/QueryManager.ts#L1226-L1241
Copy link
Contributor

@alessbell alessbell left a comment

Choose a reason for hiding this comment

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

🎉

@benjamn benjamn removed this from the v3.x.x patch releases milestone Feb 6, 2023
@benjamn benjamn merged commit 1d13de4 into main Feb 6, 2023
@benjamn benjamn deleted the issue-10262-Concast-sources-bug branch February 6, 2023 21:17
@github-actions github-actions bot mentioned this pull request Feb 6, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants