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

Fix error locations for .graphql files and tagged strings with leading newlines #122

Merged
merged 1 commit into from
May 1, 2018

Conversation

dfreeman
Copy link
Contributor

@dfreeman dfreeman commented Apr 4, 2018

Thanks for maintaining this plugin! I ran into some odd error locations when using it with raw .graphql files, and it looks like the ultimate culprit was an off-by-one bug 🙂

  • 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 pass
  • Update CHANGELOG.md with your change
  • If this was a change that affects the external API, update the README

Prior to this change, the contents of tagged strings were having leading and trailing whitespace trimmed before being processed. This masked an off-by-one error in the location calculation for errors after the first line of a document, since the most common usage pattern has a single extra leading line before the actual content which offsets that:

const query = gql`
  query ...
`;

This meant that standalone .graphql files had all their errors displayed on the wrong line (unless they opened with a single blank line as well), and tagged strings with multiple newlines at their start would also have issues. This patch removes the .trim() call and updates the line/column calculations to report the correct location.

@apollo-cla
Copy link

@dfreeman: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@dfreeman
Copy link
Contributor Author

I've rebased on top of the changes that have landed on master since I originally opened this PR—is there anything else I need to do to make sure this is good for review? Thanks!

@rwe
Copy link
Contributor

rwe commented Apr 28, 2018

I'm just a once-in-a-while contributor, not a maintainer, but this change looks good to me. CC @jnwng

@jnwng
Copy link
Contributor

jnwng commented May 1, 2018

thank you! (and thanks for the review @erydo)

@jnwng jnwng merged commit 5fc6a8c into apollographql:master May 1, 2018
@dfreeman dfreeman deleted the correct-line-numbers branch May 1, 2018 19:40
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.

None yet

4 participants