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

Broken number interpolation #4

Closed
jaydenseric opened this issue May 26, 2020 · 9 comments
Closed

Broken number interpolation #4

jaydenseric opened this issue May 26, 2020 · 9 comments
Assignees
Labels
bug Something isn't working

Comments

@jaydenseric
Copy link

This input:

const ONE = 1
const query = /* GraphQL */ `
  {
    a(b: ${ONE} c: true)
  }
`

Incorrectly results in this output:

const ONE = 1;
const query = `{a(b:${ONE}c:true)}`;

Which results in a query with invalid syntax:

{a(b:1c:true)}

This went unnoticed in my project for some time. GraphQL.js v15 no longer tolerates numbers being followed immediately with letters, and now the compressed queries are being rejected as having syntax errors:

I mistakenly thought at first this was a graphql-query-compress issue (rse/graphql-query-compress#14); but it turns out to definitely be an issue with this babel plugin.

@frontendr
Copy link
Owner

Thanks for the report! Haven't had a project with this GraphQL version so haven't noticed it.

I'll have a look when I can.

@frontendr frontendr self-assigned this May 26, 2020
@frontendr frontendr added the bug Something isn't working label May 26, 2020
frontendr added a commit that referenced this issue May 26, 2020
Release 1.3.2

- #4 - Fixed #4 - Broken number interpolation
- Updated dependencies
@jaydenseric
Copy link
Author

I still see the issue in v1.3.2…

@jaydenseric
Copy link
Author

I think it's a problem when the arguments are on new lines:

const ONE = 1

const query = /* GraphQL */ `
  {
    a(
      a: ${ONE}
      b: true
    )
  }
`

@frontendr
Copy link
Owner

That's odd as I've literally used your test case in the unit tests:

see test case input

const ONE = 1;
const TWO = 2;
const queryWithSpace = /* GraphQL */ `{
  a(b: ${ONE} c: true)
}`;

const queryWithComma = /* GraphQL */ `{
  a(b: ${ONE}, c: true)
}`;

const queryWithTrailingVariable = /* GraphQL */ `{
  a(b: ${ONE} c: ${TWO})
}`;

Is converted to:

see test case expected output

"use strict";

const ONE = 1;
const TWO = 2;
const queryWithSpace = `{a(b:${ONE} c:true)}`;
const queryWithComma = `{a(b:${ONE},c:true)}`;
const queryWithTrailingVariable = `{a(b:${ONE} c:${TWO})}`;

I'll add another test with newlines

@frontendr frontendr reopened this May 26, 2020
@frontendr
Copy link
Owner

frontendr commented May 26, 2020

Just checked and that also works:

const queryWithNewlines = /* GraphQL */ `{
  a(
    b: ${ONE}
    c: true
  )
}`;

Is converted to:

const queryWithNewlines = `{a(b:${ONE} c:true)}`;

Edit: See this commit

@jaydenseric
Copy link
Author

Here is the source:

Screen Shot 2020-05-26 at 8 20 46 pm

Here is the result:

fragment userArtworkConnection on UserArtworkConnection{page(cursor:$cursorArtwork limit:8ascending:false){hasNextPage cursorLast edges{node{id title price{format asCurrency{format(locale:"en-US")}}sold image{srcWebp:url(width:250height:250devicePixelRatio:3format:WEBP resourceHints:{preload:true})srcFallback:url(width:250height:250devicePixelRatio:3format:JPG)}}}}}fragment userGalleryConnection on UserGalleryConnection{page(cursor:$cursorGallery limit:8ascending:false){hasNextPage cursorLast edges{node{id name location{map(width:250height:150zoom:14pitch:55style:"mapbox/streets-v11" resourceHints:{preload:true})}}}}}query($userId:ID!,$cursorArtwork:ID,$cursorGallery:ID){user:node(id:$userId){... on User{name description{html(topHeadingLevel:2)}country{name emoji}region{name}gravatar(size:450,resourceHints:{preload:true})dateRegistered{relative}isAuthenticatedUser artworks{count ...userArtworkConnection}galleries{count ...userGalleryConnection}}}}

@jaydenseric
Copy link
Author

It's working now! It was probably just some tooling aggressively caching Babel config or something. Pretty strange.

@frontendr
Copy link
Owner

I can only see the first part of that but

const query = /* GraphQL */ `
  fragment a on b {
    page(
      c: $var
      d: ${ONE}
      e: false
    ) {
      hasNextPage
    }
  }
`;

Is converted to:

const query = `fragment a on b{page(c:$var d:${ONE} e:false){hasNextPage}}`;

Are you sure you've upgraded correctly?

@frontendr
Copy link
Owner

It's working now! It was probably just some tooling aggressively caching Babel config or something. Pretty strange.

Oh *pfew! Was starting to doubt myself 😄

Thanks again for the report!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants