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

7843 add context update #11339

Closed

Conversation

Troyblants
Copy link

@Troyblants Troyblants commented Nov 3, 2023

Checklist:

  • If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
  • If this PR is a new feature, please reference an issue 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

@apollo-cla
Copy link

@Troyblants: 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/

Copy link

netlify bot commented Nov 3, 2023

‼️ Deploy request for apollo-client-docs rejected.

Name Link
🔨 Latest commit 4ca6362

Copy link

changeset-bot bot commented Nov 3, 2023

⚠️ No Changeset found

Latest commit: 4ca6362

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

@phryneas
Copy link
Member

phryneas commented Nov 6, 2023

Related issue: #7843


Hi @Troyblants!
Thank you for the PR!

I like the genereral "core" direction you are going here, (adding a third argument with context and variables to the mutation's update method), but I have to be honest: these TypeScript changes are a bit too much.

Generally, we probably need to move away from generic declarations like this, as they are more akin to an any-cast than actually providing any type safety (you could call mutate 20 times with completely different contexts, and the Link chain would not know to make anything of at least 19 of them).

I know, a lot of work has gone into adding those generics everywhere (and I'm really sorry to ask this of you 😞), but could you please do the PR without them?

@jerelmiller
Copy link
Member

Hey @Troyblants 👋

Just wanted to check in and see if you're still interested in moving this PR forward! I know you've done a ton of work in this PR and just wanted to give a gentle nudge here. Thanks!

@jerelmiller
Copy link
Member

Hey @Troyblants 👋

Due to lack of activity, I'm going to go ahead and close this PR. Feel free to reopen if you're interested in seeing this change through. Thanks again for the contribution!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants