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

Multiple fragments support #895

Merged
merged 9 commits into from
Jul 31, 2018
Merged

Multiple fragments support #895

merged 9 commits into from
Jul 31, 2018

Conversation

maticzav
Copy link
Contributor

TODO:

  • If this PR is a new feature, 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
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change. Include a description of your change, link to PR (always) and issue (if applicable). Add your CHANGELOG entry under vNEXT. Do not create a new version number for your change yourself.

This is the implementation of #894 issue.

@apollo-cla
Copy link

@maticzav: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@ghost ghost added the feature New addition or enhancement to existing solutions label Jul 17, 2018
@maticzav
Copy link
Contributor Author

cc @freiksenet

freiksenet
freiksenet suggested changes Jul 19, 2018
View reviewed changes
Copy link
Contributor

@freiksenet freiksenet left a comment

Choose a reason for hiding this comment

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

Please fix the changelog reformatting. Otherwise some minor issues.

CHANGELOG.md Outdated
@@ -1,341 +1,345 @@
# Change log
Copy link
Contributor

Choose a reason for hiding this comment

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

Changelog got fully reformatted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed ✅

src/transforms/ReplaceFieldWithFragment.ts
@@ -119,3 +129,30 @@ function parseFragmentToInlineFragment(

throw new Error('Could not parse fragment');
}

function concatInlineFragments(
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, few issues here.

  • If fragments have conflicting aliases this will break
  • Maybe this should dedupe fields (also watch out for aliases in that case)

@maticzav maticzav mentioned this pull request Jul 19, 2018
Open
3 tasks
@freiksenet
Copy link
Contributor

This looks good to me.

@stubailo stubailo merged commit c8e8b01 into ardatan:master Jul 31, 2018
@stubailo
Copy link
Contributor

Fixed conflicts and merged via CLI, thank you!

@yaacovCR yaacovCR mentioned this pull request Apr 1, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@freiksenet freiksenet freiksenet approved these changes

Assignees
No one assigned
Labels
feature New addition or enhancement to existing solutions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maticzav @apollo-cla @freiksenet @stubailo