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

Add tests for "swift: Fix --operationIdsPath option" #1431 #2017

Merged
merged 5 commits into from
Jul 15, 2020

Conversation

manicmaniac
Copy link
Contributor

@manicmaniac manicmaniac commented Jun 26, 2020

I took over the rest tasks (adding tests) of #1431.
Could you make sure that it is enough?

  • Rebased onto the current HEAD of master branch
  • Modified existing tests and updated snapshots
  • Confirmed the tests passed in my local env
    • However it looks flaky so I ran tests with node node_modules/.bin/jest -t generate -i --no-cache to exclude unchanged.

TODO:

  • Update CHANGELOG.md* with your change (include reference to issue & this PR)
  • 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

*Make sure changelog entries note which project(s) has been affected. See older entries for examples on what this looks like.

@manicmaniac manicmaniac force-pushed the add-tests-for-pr-1431 branch from a131cc1 to 2e5338a Compare June 29, 2020 04:47
Copy link
Contributor

@designatednerd designatednerd left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me but would love a look from @JakeDawkins, who actually knows how javascript works, unlike me 😆

@@ -267,7 +267,7 @@ export class SwiftAPIGenerator extends SwiftGenerator<CompilerContext> {
);
fragmentsReferenced.forEach(fragmentName => {
this.print(
swift`.appending(${this.helpers.structNameForFragmentName(
swift`.appending("\\n" + ${this.helpers.structNameForFragmentName(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@designatednerd Sorry for the additional commit after receiving LGTM.

I just found that the current generated code for Swift doesn't have a newline between query and fragment.
Because of missing newlines, the generated operation ID and sent query's sha256 hash always differs.
After this change, operation ID and sha256 will be same so we can treat Persisted Query more easily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current implementation outputs the following query document as source property of operationIds.json.
sha256 hash calculated from this source is put into the same operationIds.json.

query SomeQuery {
    ...SomeFragment
}
fragment SomeFragment {
    id
}

However, Apollo iOS sends the following query document (missing newlines).
sha256 hash is calculated from this source at runtime so there's difference between 2 hashes.

query SomeQuery {
    ...SomeFragment
}fragment SomeFragment {
    id
}

This commit fixes this problem with appending missing newlines.

@manicmaniac
Copy link
Contributor Author

@JakeDawkins Could you take a look at this one when you have time?

Copy link
Contributor

@JakeDawkins JakeDawkins left a comment

Choose a reason for hiding this comment

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

@manicmaniac this looks good to me! Thanks for your patience here! :)

@JakeDawkins JakeDawkins merged commit 9bb4b3f into apollographql:master Jul 15, 2020
@manicmaniac manicmaniac deleted the add-tests-for-pr-1431 branch July 15, 2020 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants