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

apollo client:push update output to include more details #1290

Merged
merged 6 commits into from
May 23, 2019

Conversation

evans
Copy link
Contributor

@evans evans commented May 22, 2019

This updates the operation registration command to use the new resolver
that returns more information, namely added operations and invalidation
messages associated with the operation. The apollo client:push command
then outputs this information

Happy path:
image

Sad path:
image

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.

This updates the operation registration command to use the new resolver
that returns more information, namely added operations and invalidation
messages associated with the operation. The `apollo client:push` command
then ouputs this information
@trevor-scheer
Copy link
Member

trevor-scheer commented May 22, 2019

Fixes #1272? It would be great to get @mgeraci 's feedback on this - he suggested this idea to me fairly recently while we stepped through an issue they faced.

Edited to remove unintended tone (possibly snarky 😬), apologies if this was interpreted with anything but a neutral/positive attitude. Thanks @evans!

@evans
Copy link
Contributor Author

evans commented May 22, 2019

@trevor-scheer That's a great idea! I'll see how to work that into the PR

packages/apollo/src/commands/client/push.ts Outdated Show resolved Hide resolved
} client to the ${serviceName} service in Engine`
);
if (!registrationSuccess) {
if (invalidOperations) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible for !registrationSuccess && !invalidOperations? I only ask in case it's a possible scenario, and if it has any semantics that we can communicate.

I don't imagine so, but I'd suggest that we throw or log something if that's the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now it shouldn't be possible, though maybe in the future. I'll add an error message!

}
} else {
if (newOperations && newOperations.length) {
task.title = `Added ${
Copy link
Member

Choose a reason for hiding this comment

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

Small nit: Added 6 to Apollo Platform reads a bit awkward. I'll offer some suggestions, but please feel free to disregard / use your discretion.

Successfully {registered|pushed|added} 6 operations (optional: to the operation {registry|safelist})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it!

@evans evans merged commit 661df7e into master May 23, 2019
@evans evans deleted the evans/update-client-push branch May 23, 2019 00:47
@mgeraci
Copy link

mgeraci commented May 23, 2019

Sorry I didn't get in here before the merge, but this looks like a great set of improvements! Did it end up hitting #1272 — there are tons of great messaging improvements here, but I wasn't sure if this change also errors out if it finds an invalid operation. Either way, great work!

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.

3 participants