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

(processVariables: true) do not collect input if variable is missing #456

Conversation

dimatill
Copy link
Contributor

@dimatill dimatill commented Oct 5, 2022

Fix for the case when an optional variable is not provided

@changeset-bot
Copy link

changeset-bot bot commented Oct 5, 2022

🦋 Changeset detected

Latest commit: abd8362

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@graphql-hive/client Patch

Not sure what this means? Click here to learn what changesets are.

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

Comment on lines 345 to 363
const info = collect(
parse(/* GraphQL */ `
query getProjects($pagination: PaginationInput, $type: ProjectType!) {
projects(filter: { pagination: $pagination, type: $type }) {
id
}
}
`),
{
type: 'STITCHING',
}
).value;

expect(info.fields).toContain(`FilterInput.type`);
expect(info.fields).toContain(`FilterInput.pagination`);
expect(info.fields).not.toContain(`PaginationInput.limit`);
expect(info.fields).not.toContain(`PaginationInput.offset`);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm, if it's not collecting PaginationInput then when PaginationInput is removed (because it's safe from the usage reports perspective) this query will fail, no? Because it contains PaginationInput and it's not defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, yes, you're right. I need to think about it and find a way to handle this case

@dimatill
Copy link
Contributor Author

dimatill commented Oct 5, 2022

@kamilkisiela , I just pushed new changes. I didn't test anything yet, but it looks to me like it SHOULD be ok this way both from UI and GraphQL Inspector perspectives. Just want to hear your opinion before going forward.

@dimatill
Copy link
Contributor Author

dimatill commented Oct 9, 2022

@kamilkisiela I've tested it manually with a local instance of Hive and it looks good to me.
The test was done with the same schema as in usage-collector.spec.ts.
The test scenario was the same as in (processVariables: true) should not collect input when corresponding variable is not provided test case.

Please check attached screenshots.
Here is a description of screenshots:

In the UI I can see that PaginationInput was used 1 time while all fields were used zero times. This could be a little bit confusing, but this is what actually happened: fields weren't used but the input itself was used in the query definition.

hive schema:check also works as expected: removing FilterInput.order and ProjectOrderByInput is considered a non-breaking change because they weren't used. Removing PaginationInput and FilterInput.pagination is considered a breaking change because they were used in the query.

What do you think? Can we merge this PR?

Screenshot 2022-10-09 at 17 50 25

Screenshot 2022-10-09 at 18 07 37

@dimatill dimatill requested a review from kamilkisiela October 9, 2022 16:32
@kamilkisiela kamilkisiela force-pushed the do_not_collect_input_for_missing_variable branch from 2425ec7 to abd8362 Compare October 20, 2022 07:58
@kamilkisiela kamilkisiela merged commit fb9b624 into graphql-hive:main Oct 25, 2022
@kamilkisiela
Copy link
Contributor

Thank you very very very much

@kamilkisiela
Copy link
Contributor

I will deploy it to production tomorrow

@dimatill
Copy link
Contributor Author

woohoo, thank you @kamilkisiela !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants