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

Cover batched XHR cancelling with tests #1742

Merged
merged 2 commits into from
Oct 24, 2022

Conversation

PowerKiKi
Copy link
Collaborator

@PowerKiKi PowerKiKi commented Dec 28, 2021

Checklist:

  • If this PR is a new feature, please reference a discussion where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests
  • Try to include the Pull Request inside of CHANGELOG.md

This is dependent on apollographql/apollo-client#9248, so it is not strictly specific to apollo-angular, but it seemed important to ensure that this package does indeed allow to cancel XHR even when batching is enabled.

There is no changelog entries, because there not change at all for end-users.

It should only be merged after a new apollo-client is released (and after updating our lock file).

@kamilkisiela kamilkisiela reopened this Feb 16, 2022
@PowerKiKi PowerKiKi marked this pull request as ready for review April 28, 2022 19:55
@PowerKiKi
Copy link
Collaborator Author

Now that @apollo/client 3.6.0 has been released, this PR can be reviewed and merged too.

@PowerKiKi
Copy link
Collaborator Author

@kamilkisiela since this is mostly adding new test cases, it should be relatively fast to review. Do you think it could get merged ?

@PowerKiKi
Copy link
Collaborator Author

Rebased.

Because CI pipeline changed after this PR was created it looks like some jobs are still pending. But actually everything that need to pass does indeed pass.

@kamilkisiela would you have an opportunity to review this ?

@PowerKiKi
Copy link
Collaborator Author

@kamilkisiela since you spent a bit of time on this lib recently, maybe you'd have time to review this (rebased) PR ?

This is dependent on apollographql/apollo-client#9248,
so it is not strictly specific to `apollo-angular`, but it seemed important to
ensure that this package does indeed allow to cancel XHR even when batching is
enabled.
@kamilkisiela
Copy link
Owner

@PowerKiKi do you want to maintain the library? :)

@kamilkisiela
Copy link
Owner

say Yes and I click "merge" :)

@PowerKiKi
Copy link
Collaborator Author

Yes 😃

But I'd like to ask a few you question before if you actually step down from being a maintainer. Such as why did you recently re-introduce ApolloModule ? and why would you step down ?

@kamilkisiela
Copy link
Owner

@PowerKiKi do you use Slack? If so, are you okay with joining The Guild's #apollo-angular channel? It would be easier to discuss things.

In short, I'm not stepping down, it's just that I'm too busy with other things and this project needs some love.
ApolloModule was re-introduced to enable back the lazy loading. Also, using APOLLO_OPTIONS was out of sync with initialization of Apollo service, same for APOLLO_FLAGS.

@PowerKiKi
Copy link
Collaborator Author

I don't usually use slack, but I will if necessary. I can't find the URL for Slack anywhere though....

@kamilkisiela
Copy link
Owner

I need to invite you, it's not a public slack. Can share your email address with me, over email (to not expose it here)?

@kamilkisiela kamilkisiela enabled auto-merge (squash) October 24, 2022 10:29
@kamilkisiela kamilkisiela disabled auto-merge October 24, 2022 10:29
@kamilkisiela kamilkisiela enabled auto-merge (squash) October 24, 2022 10:29
@kamilkisiela kamilkisiela merged commit ed70140 into kamilkisiela:master Oct 24, 2022
@PowerKiKi PowerKiKi deleted the batch-cancel branch October 24, 2022 12:53
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