Skip to content

feat(visitor-plugin-common): Simpler and more readable output when using inlineFragmentTypes: combine (resolve #8320)#8321

Open
yusufkandemir wants to merge 7 commits into
dotansimha:masterfrom
yusufkandemir:feat-remove-redundant-typename-on-combined-fragments
Open

feat(visitor-plugin-common): Simpler and more readable output when using inlineFragmentTypes: combine (resolve #8320)#8321
yusufkandemir wants to merge 7 commits into
dotansimha:masterfrom
yusufkandemir:feat-remove-redundant-typename-on-combined-fragments

Conversation

@yusufkandemir

Copy link
Copy Markdown

Description

Related #8320. See the issue for a detailed explanation.

Type of change

  • New feature (non-breaking change which adds functionality)

(One could say this is a bug fix and not a feature, depends on how you look at it)

Screenshots/Sandbox (if appropriate/relevant):

See #8320 for screenshots

How Has This Been Tested?

By updating and improving the existing test case for inlineFragmentTypes: combine. There are basically 3 possible combinations:

  • Some fragments on a single type query: Avoid extra __typename
  • No fragments on a query: Preserve the behavior and make sure __typename is added
  • Some fragments on a union type query: Avoid extra __typename for types that are covered by a fragment

Test Environment:

  • OS: macOS Monterey 12.5
  • NodeJS: v16.16.0

Checklist:

  • I have followed the CONTRIBUTING doc and the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • (N/A) I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • (N/A) Any dependent changes have been merged and published in downstream modules

Further comments

None.

Also improved the comments and extracted to a method to increase clarity
After a bit of fiddling around I discovered that buildSelectionSet gets
run for each possible type in union typed queries.
This means each fragment will match the main type. So, we can simply
check if there is at least one fragment.
I actually implemented the test cases earlier, but 🤷
@changeset-bot

changeset-bot Bot commented Sep 3, 2022

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 5c5b6ea

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@yusufkandemir yusufkandemir changed the title refactor: Remove typename when not needed for combine (resolve #8320) [visitor-plugin-common] feat: Avoid extra __typename when using inlineFragmentTypes: combine (resolve #8320) Sep 3, 2022
@yusufkandemir yusufkandemir changed the title [visitor-plugin-common] feat: Avoid extra __typename when using inlineFragmentTypes: combine (resolve #8320) feat(visitor-plugin-common): Simpler and more readable output when using inlineFragmentTypes: combine (resolve #8320) Sep 3, 2022
@saihaj

saihaj commented Oct 19, 2023

Copy link
Copy Markdown
Collaborator

can you please rebase and run yarn changeset and create a minor changeset explaining the changes?

@saihaj saihaj added the waiting-for-answer Waiting for answer from author label Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-for-answer Waiting for answer from author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants