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

Add ability to chain rest links #309

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

willyboy
Copy link

POC PR for chaining Rest Links

If a Rest Link directive has an endpoint defined but the current Rest Link does not have a matching endpoint registered, don't remove that directive. Assume it will be defined later down the link chain.

Caveats:

  1. Only endpoints can be used because that's how we know what directives to keep/remove. We could figure out another toggle/option to control this. If a URI was used, everything with an undefined endpoint would default to it and the feature wouldn't work.
  2. This is POC code. I didn't add any tests, docs, etc.
  3. This code doesn't currently protect against an endpoint being undefined once getting to the end of the Link chain

Issue reference: #308

If a Rest Link directive has an endpoint defined but the current Rest Link does not have a matching endpoint registered, don't remove that directive. Assume it will be defined in the following rest link.
@apollo-cla
Copy link

@willyboy: 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
Collaborator

@fbartho fbartho left a comment

Choose a reason for hiding this comment

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

Conceptually I like the approach!

I think I'd like an option-flag that declares enableRestLinkChaining (or isTerminalLink + default: true), that way this doesn't have to be a breaking change for the whole link, but rather just becomes a feature release.

Any thoughts?

src/restLink.ts Outdated Show resolved Hide resolved
src/restLink.ts Outdated Show resolved Hide resolved
@willyboy
Copy link
Author

I'm leaning towards enableChaining. I like the feature flag nature of it so that if chaining became the default, it could be more easily deprecated.

@willyboy willyboy closed this Jul 13, 2023
@willyboy willyboy reopened this Jul 13, 2023
willyboy and others added 4 commits July 13, 2023 15:29
Co-authored-by: Frederic Barthelemy <github@fbartho.com>
Co-authored-by: Frederic Barthelemy <github@fbartho.com>
@willyboy
Copy link
Author

Made some updates. If this works I can go ahead and add a unit test and an example in the docs.

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.

None yet

3 participants