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.

Sign up for GitHub

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

Jump to bottom

Avoid duplicate cacheControl directives via isDirectiveDefined #2428

Merged
merged 5 commits into from May 22, 2019
Merged

Avoid duplicate cacheControl directives via isDirectiveDefined #2428

merged 5 commits into from May 22, 2019

Conversation

cheapsteak
Copy link
Member

@cheapsteak cheapsteak commented Mar 11, 2019 •
edited

This PR adds a utility function isDirectiveDefined and uses it to guard against duplicate directives when the user's schema already contains a directive (in this case, cacheControl)

TODO:

  • Update CHANGELOG.md with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

@cheapsteak cheapsteak changed the title Avoid duplicate directives Avoid duplicate cacheControl directive Mar 11, 2019
@akadop
Copy link

akadop commented Mar 11, 2019

Thanks for working on this!

@glasser
Copy link
Member

glasser commented Mar 26, 2019

I also think the text of the cache control directive should be exported by apollo-cache-control

@stevez86 stevez86 mentioned this pull request May 20, 2019
Closed
@stevez86
Copy link

Any chance this will be merged anytime soon?

@abernix abernix self-assigned this May 21, 2019
cheapsteak and others added 4 commits May 22, 2019 23:00
@cheapsteak @abernix
Add util isDirectiveDefined, and tests
167060f
@cheapsteak @abernix
Prevent duplicate directive from being added
c41d260
@cheapsteak @abernix
update changelog
01bccc2
@abernix
Switch to importing directly from graphql/language.
83e49cb 
This is me being nitty but this helps me understand the department that the
import comes from rather than the globby export-catch-all that is the `main`
module. :)
@abernix abernix force-pushed the chang/avoid-duplicate-directives branch from f4ab679 to 6015761 Compare May 22, 2019 20:00
@abernix abernix changed the base branch from master to release-2.6.0 May 22, 2019 20:00
@abernix abernix added this to the Release 2.6.0 milestone May 22, 2019
@abernix abernix force-pushed the chang/avoid-duplicate-directives branch from 6015761 to ab282e2 Compare May 22, 2019 20:05
@abernix
Use Kind.DIRECTIVE_DEFINITION constant rather than string literal.
6ac98dc 
This is just for stability.  I know we don't do it everywhere right now, but
we should!

This also removes the extra function, but I don't think we're really
getting too much from that now, or later.  The constant comparison should be
the ideal way of doing this, and TypeScript should be able to do the type
narrowing on its own.
@abernix abernix force-pushed the chang/avoid-duplicate-directives branch from ab282e2 to 6ac98dc Compare May 22, 2019 20:07
@abernix abernix changed the title Avoid duplicate cacheControl directive Avoid duplicate cacheControl directives via isDirectiveDefined May 22, 2019
@abernix abernix merged commit 4e84def into release-2.6.0 May 22, 2019
@cheapsteak cheapsteak deleted the chang/avoid-duplicate-directives branch May 22, 2019 22:48
@stevez86
Copy link

I didn't realize that I could generate my own schema and get around this, you might want to add something in the documentation

abernix added a commit that referenced this pull request May 31, 2019
@abernix
Revert "Avoid duplicate cacheControl directives via `isDirectiveDef…
c742335 
…ined` (#2428)"

This reverts commit 4e84def.
abernix added a commit that referenced this pull request May 31, 2019
@abernix
Add CHANGELOG.md for breaking change inadvertently introduced in 2.6.0.
6adec0a 
Also added the MISSING `CHANGELOG.md` from the original release which I
somehow stomped out!

Ref: #2428
@abernix
Copy link
Member

abernix commented May 31, 2019

This caused the regression in #2753, which is best explained in the body of #2754, which contains its reversion.

@cheapsteak Not sure if you'll have the opportunity to revisit, but just letting you know! (Also as noted above, passing your own schema is a work-around, so potentially less urgent!)

cheapsteak added a commit that referenced this pull request Jun 1, 2019
@cheapsteak
Revert "Revert "Avoid duplicate cacheControl directives via `isDire…
e2ed33f 
…ctiveDefined` (#2428)""

This reverts commit c742335.
@cheapsteak cheapsteak mentioned this pull request Jun 1, 2019
Merged
cheapsteak added a commit that referenced this pull request Jun 24, 2019
@cheapsteak
Revert "Revert "Avoid duplicate cacheControl directives via `isDire…
b36d00b 
…ctiveDefined` (#2428)""

This reverts commit c742335.
abernix pushed a commit that referenced this pull request Jun 25, 2019
@cheapsteak @abernix
Avoid duplicate cacheControl directives via `isDirectiveDefin… (#2762)
65ca915 
* Revert "Revert "Avoid duplicate `cacheControl` directives via `isDirectiveDefined` (#2428)""

This reverts commit c742335.

* Account for typeDefs possibly being a string

* Add tests to ensure isDirectiveDefiend works for strings

* rename test file to add ".test"

* update changelog
@abernix abernix mentioned this pull request Aug 23, 2019
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@abernix abernix abernix approved these changes

@martijnwalraven martijnwalraven Awaiting requested review from martijnwalraven

Assignees

@abernix abernix

Labels
None yet
Projects
None yet
Milestone
Release 2.6.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@cheapsteak @akadop @glasser @stevez86 @abernix