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 escaping of literal .graphql files #88

Merged
merged 2 commits into from
Sep 7, 2017

Conversation

lydell
Copy link
Contributor

@lydell lydell commented Aug 26, 2017

TODO:

  • 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

const expected = 'ESLintPluginGraphQLFile`query { someValueWith\\` }`';
it('should wrap with backticks, escape properly and prepend internalTag', () => {
const query = 'query { search(q: "` \\n ${}") { title } }';
const expected = 'ESLintPluginGraphQLFile`query { search(q: "\\` \\\\n \\${}") { title } }`';
Copy link
Contributor

Choose a reason for hiding this comment

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

as a general comment i think this addition would be clearer as its own test case — im having some trouble parsing what's actually being added here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I did here is:

  • Updated the test description. This could have been done as a separate commit – the first commit, since the updated test description is valid for the old test as well.
  • Beefed up the test case with more stuff that needs escaping than backticks.

So I don't see how a separate test case would make sense :)

Copy link
Contributor

Choose a reason for hiding this comment

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

just to double-check here - the \\n is getting escaped as \\\\n but is \\n the already escaped version of \n?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Welcome to backslash hell :) I have dealt with this in several projects such as CoffeeScript and Prettier :)

The real input is:

query { search(q: "` \n ${}") { title } }

and the real output is:

ESLintPluginGraphQLFile`query { search(q: "\` \\n \${}") { title } }`

@lydell
Copy link
Contributor Author

lydell commented Aug 29, 2017

Btw, this PR is about making a template literal correctly (with correct escaping) for correctness sake. Things do break if someone has "${}" somewhere in a .graphql file right now (which is valid GraphQL, but a bit unlikely), and this PR fixes it.

@jnwng jnwng merged commit 45ecae7 into apollographql:master Sep 7, 2017
@lydell lydell deleted the literal-escaping branch September 8, 2017 05:05
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.

2 participants