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

Named fragment support. #251

Merged
merged 12 commits into from
Jun 4, 2016
Merged

Named fragment support. #251

merged 12 commits into from
Jun 4, 2016

Conversation

Poincare
Copy link
Contributor

@Poincare Poincare commented May 28, 2016

Named fragment support within the client. Now passes around the fragments within a query as a symbol table (i.e. an object mapping from fragment name to the fragment definition).

TODO:

  • Update CHANGELOG.md with your change
  • 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

@Poincare
Copy link
Contributor Author

@helfer - could you review?

@zol zol added the fixed label Jun 2, 2016
@helfer helfer mentioned this pull request Jun 2, 2016
@@ -4,7 +4,7 @@ Expect active development and potentially significant breaking changes in the `0

### vNEXT
- Removed AuthTokenHeaderMiddleware code and related tests from apollo-client [Issue #247](https://github.com/apollostack/apollo-client/issues/247)
- Fixed the getFromAST functions so that they work with queries that have named fragments (i.e. working toward named fragment support)
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs an issue and PR link please!

@Poincare
Copy link
Contributor Author

Poincare commented Jun 3, 2016

Made the changes.

@@ -200,4 +202,22 @@ describe('AST utility functions', () => {
const newDoc = replaceOperationDefinition(queryWithFragments, newQueryDef);
assert.equal(print(newDoc), print(expectedNewQuery));
});

it('should create the fragment sym table correctly', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

should be renamed to fragment map as well

@helfer
Copy link
Contributor

helfer commented Jun 3, 2016

Just one tiny comment about one of the tests. Ready to merge otherwise. 🎉

@Poincare
Copy link
Contributor Author

Poincare commented Jun 3, 2016

Fixed.

@stubailo
Copy link
Contributor

stubailo commented Jun 3, 2016

Looks awesome!

@helfer
Copy link
Contributor

helfer commented Jun 4, 2016

Hm, we seem to have merged something in the meantime. Can you rebase one last time?

@Poincare
Copy link
Contributor Author

Poincare commented Jun 4, 2016

Rebased.

@helfer helfer merged commit a410ae6 into master Jun 4, 2016
@helfer helfer removed the fixed label Jun 4, 2016
@helfer
Copy link
Contributor

helfer commented Jun 4, 2016

Merged!

@Poincare Poincare deleted the named_fragments branch June 4, 2016 06:13
jbaxleyiii pushed a commit that referenced this pull request Oct 17, 2017
ramusus added a commit to cinemanio/frontend that referenced this pull request Jun 19, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 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