isographlabs / isograph

The UI framework for teams that move fast — without breaking things.
MIT License
202 stars 9 forks source link

Numeric literals are generated as strings in normalization ASTs #24

Closed rbalicki2 closed 3 months ago

rbalicki2 commented 4 months ago
      pullRequests(last: 1) {
        totalCount
      }

results in

      {
        kind: "Linked",
        fieldName: "pullRequests",
        alias: null,
        arguments: [
          [
            "last",
            { kind: "Literal", value: "1" },
          ],
        ],
        selections: [
          {
            kind: "Scalar",
            fieldName: "totalCount",
            alias: null,
            arguments: null,
          },
        ],
      },

This maybe isn't broken, because the literal value is stringified anyway, but it probably should be generated as a number in the AST.

ismajl-ramadani commented 3 months ago

Can I work on this?

rbalicki2 commented 3 months ago

Yes, absolutely! Let me know whether you would like to touch base, happy to walk you through what roughly needs to happen and get you acquainted with the codebase. The base place to reach out is on the discord https://discord.gg/kDCcN3EDR6 (the #isograph channel on the GraphQL community discord)

ismajl-ramadani commented 3 months ago

@rbalicki2 I managed to run the examples and also use the local build of the cli to generate artifacts. Development workflow section on docs was quite informative.

I need to look more into this as I'm trying to get familiar with the repo and the concepts but I think that the issue might be inside get_serialized_field_arguments, on the NonConstantValue::Integer(int_value) => { arm of the match.

...
{indent_2}{{ kind: \"Literal\", value: \"{int_value}\" }},\n\
...

Removing double quotes seems to generate int literal correctly for the ast, but I'm not sure if this will brake anything else. Didn't have much time, but will get back to this. Also if you can give me any other hint, it would be awesome.

Love the project btw!!!

rbalicki2 commented 3 months ago

Thank you!!

Yes, I think that should do it. For some manual testing, please make sure:

  field Query.PetDetailRoute($id: ID!) @component {
    foo: pet(id: $id) {
      id
    }
    pet(id: 0) {
      name
      PetCheckinsCard
      PetBestFriendCard
      PetPhraseCard
      PetTaglineCard
    }
  }

and make sure that you can click on the first pet (Makayla) and see data.

If this all looks good to you, we can get this landed!

ismajl-ramadani commented 3 months ago

Thanks for the additional help. I've tested it with the pet-demo as you suggested and it seems to work. I created a PR and added the small fix and also generated artifacts for the github-demo. Other demos looks like they don't have any integer literal, so they should be fine.

Please let me know if I have missed something.