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

Feature/remove unused @client meta #3482

Merged

Conversation

justinmakaila
Copy link
Contributor

@justinmakaila justinmakaila commented May 19, 2018

Checklist:

  • 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
  • If this was a change that affects the external API used in GitHunt-React, update GitHunt-React and post a link to the PR in the discussion.

In reference to #3475... Wish I could say a consensus was reached, but there was no activity for days and this is mission critical stuff... the client is currently spitting out invalid queries after manipulating the operations...

…tadata, such as variables and fragment spreads. Added test coverage for the individual features (get/remove arguments/fragment spreads) as well as the integration point.
 - apollo-boost@0.1.7-0
 - apollo-cache-inmemory@1.2.2-0
 - apollo-cache@1.1.9-0
 - apollo-client@2.3.2-0
 - apollo-utilities@1.0.13-0
 - graphql-anywhere@4.1.11-0
 - apollo-boost@0.1.7-1
 - apollo-cache-inmemory@1.2.2-1
 - apollo-cache@1.1.9-1
 - apollo-client@2.3.2-1
 - apollo-utilities@1.0.13-1
 - graphql-anywhere@4.1.11-1
 - apollo-boost@0.1.7-2
 - apollo-cache-inmemory@1.2.2-2
 - apollo-cache@1.1.9-2
 - apollo-client@2.3.2-2
 - apollo-utilities@1.0.13-2
 - graphql-anywhere@4.1.11-2
@ghost ghost added blocking labels May 19, 2018
@justinmakaila
Copy link
Contributor Author

Trying to get apollo-utilities hooked up to my project repo, advice around that would be great...

@apollo-cla
Copy link

apollo-cla commented May 19, 2018

Fails
🚫

No CHANGELOG added.

Warnings
⚠️

❗ Big PR

Generated by 🚫 dangerJS

@ghost ghost added blocking labels May 19, 2018
@hwillson hwillson changed the title Feature/remove unused @client meta [WIP] Feature/remove unused @client meta May 29, 2018
@ghost ghost added blocking labels May 29, 2018
@hwillson
Copy link
Member

hwillson commented Jun 6, 2018

Thanks for submitting this PR @justinmakaila! Unfortunately, this isn't something we're ready to accept. The changes you're proposing are fairly significant and have far reaching affects across the apollo-client codebase. I think we need to step back here a bit, and discuss the best way forward back in #3475. I realize there hasn't been much traction in the issue thread yet, but it would definitely help if you could supply a small runnable reproduction that shows this issue happening (with the latest version of apollo-client), as a starting point. That will help move things along. I'll close this PR for now, but we can always re-open it in the future if/as needed. Thanks again for taking the time to work on this!

@hwillson
Copy link
Member

hwillson commented Jun 7, 2018

Re-opening this to take another look (as discussed with @justinmakaila via Slack).

@hwillson hwillson reopened this Jun 7, 2018
@hwillson hwillson self-assigned this Sep 20, 2018
@codecov
Copy link

codecov bot commented Sep 25, 2018

Codecov Report

Merging #3482 into master will decrease coverage by 3.82%.
The diff coverage is 79.86%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3482      +/-   ##
=========================================
- Coverage   89.13%   85.3%   -3.83%     
=========================================
  Files          47      41       -6     
  Lines        2679    2573     -106     
  Branches      609     609              
=========================================
- Hits         2388    2195     -193     
- Misses        274     370      +96     
+ Partials       17       8       -9
Impacted Files Coverage Δ
packages/apollo-utilities/src/transform.ts 84.87% <79.86%> (-10.46%) ⬇️
packages/apollo-utilities/src/fragments.ts 76.19% <0%> (-23.81%) ⬇️
packages/apollo-cache/src/cache.ts 86.11% <0%> (-13.89%) ⬇️
packages/apollo-boost/src/index.ts 76.27% <0%> (-12.44%) ⬇️
...ckages/apollo-cache-inmemory/src/recordingCache.ts 74.35% <0%> (-11.85%) ⬇️
packages/graphql-anywhere/src/utilities.ts 35.29% <0%> (-8.3%) ⬇️
packages/apollo-client/src/scheduler/scheduler.ts 89.39% <0%> (-7.44%) ⬇️
packages/apollo-client/src/data/queries.ts 89.55% <0%> (-7.38%) ⬇️
packages/apollo-utilities/src/storeUtils.ts 31.66% <0%> (-6.6%) ⬇️
packages/apollo-client/src/errors/ApolloError.ts 93.75% <0%> (-6.25%) ⬇️
... and 39 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7643b90...3b2c68e. Read the comment docs.

@hwillson
Copy link
Member

Sorry for the long delay here @justinmakaila. We're working on a new version of apollo-link-state that is tightly integrated directly into Apollo Client. As part of that work, we're re-visiting how @client is interpreted. We'll definitely review your changes here as part of that work. More details coming shortly. Thanks!

@hwillson hwillson changed the title [WIP] Feature/remove unused @client meta Feature/remove unused @client meta Nov 28, 2018
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Thanks very much for working on this @justinmakaila! Sorry for the long delay in getting this reviewed. LGTM! 👍

@hwillson hwillson merged commit e6de4f4 into apollographql:master Nov 28, 2018
hwillson added a commit that referenced this pull request Nov 28, 2018
@justinmakaila
Copy link
Contributor Author

YESSSSSSS

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2023
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