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

Update to latest version of Apollo CLI #2027

Closed
calvincestari opened this issue Nov 11, 2021 · 8 comments · Fixed by #2028
Closed

Update to latest version of Apollo CLI #2027

calvincestari opened this issue Nov 11, 2021 · 8 comments · Fixed by #2028
Assignees
Labels
codegen Issues related to or arising from code generation

Comments

@calvincestari
Copy link
Member

This is to get apollographql/apollo-tooling#2473 into codegen and align with the output from client:push for safelisting/whitelisting operations.

@calvincestari calvincestari added this to the Release 0.50.0 milestone Nov 11, 2021
@calvincestari calvincestari added 2021-11 codegen Issues related to or arising from code generation labels Nov 11, 2021
@AnthonyMDev
Copy link
Contributor

I'm going to need a spec for this safelisting functionality for the new codegen also. Just something that I can convert into unit tests to confirm that the new codegen has parity here.

@calvincestari
Copy link
Member Author

I wonder if a better test for the new codegen would be to leave a CI test that downloads the tooling and generates output we can compare to.

@AnthonyMDev
Copy link
Contributor

The output is going to be so wildly different that comparing them would be difficult. We aren’t comparing the to be the same, we are only looking for one specific piece of behavior to be correct. (Not necessarily the same, just correct behavior).

Unless I’m misunderstanding something!

@designatednerd
Copy link
Contributor

The output on our end would be the raw query which gets sent to the server, with __typename added wherever it needs to be. That's what gets hashed and compared for safelisting.

@calvincestari
Copy link
Member Author

The output on our end would be the raw query which gets sent to the server, with __typename added wherever it needs to be. That's what gets hashed and compared for safelisting.

Correct - the operationIDs.json file is the output we'd need for comparison no?

@designatednerd
Copy link
Contributor

Ideally yes

@AnthonyMDev
Copy link
Contributor

I still don't think that comparison to the old codegen output is the appropriate way to test this.

If at some point we need to alter the expected output again because of another unforeseen problem, then the outputs are going to deviate. Unit tests should directly and exclusively test the actual behavior you expect to have, not direct parity with an unmaintained legacy system.

We can revisit the conversation about that when we go to actually implement this though.

@calvincestari
Copy link
Member Author

I still don't think that comparison to the old codegen output is the appropriate way to test this.

It's not about the "old codegen" and instead about the external code that registers the queries for safelisting; that just happens to be the same CLI tooling. The operationIDs.json output is the simplest form of what we'd need for comparison, I don't think client:push outputs the formatted query anywhere we could compare to.

If at some point we need to alter the expected output again because of another unforeseen problem, then the outputs are going to deviate. Unit tests should directly and exclusively test the actual behavior you expect to have, not direct parity with an unmaintained legacy system.

Correct - this isn't a unit test. We'll have unit tests to ensure that __typename is being added where we expect it to and in addition to that I'm suggesting an integration test that ensures our expectation matches what is happening elsewhere because if we aren't proactive about catching a mismatch the outcome impacts users.

We can revisit the conversation about that when we go to actually implement this though.

👍🏻 - I've created #2029 as a TODO so we don't miss it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codegen Issues related to or arising from code generation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants