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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prettify graphql query variables #4800

Closed
wants to merge 2 commits into from

Conversation

SethFalco
Copy link
Contributor

@SethFalco SethFalco commented May 19, 2022

Before the "Prettify GraphQL" button only prettified the query, but not the variables.
Now the variables are prettified too! (While respecting the tab/indent-size settings!)

I'm honestly not really sure what it was doing with the variables before. 馃
As in, it was calling prettify on the JSON before as well, but it just wasn't updating the object in the GUI. Not sure if that was a bug or maybe I'm missing something?

Before

old

After

new


On the side this also makes 2 minor edits in the context of this PR:

  • Trim whitespace on one of the fixtures.
  • Update the link to JSON Lint (and use the correct capitalization) as the domain expired and was bought by someone else it seems. 馃

@SethFalco SethFalco marked this pull request as draft May 19, 2022 21:36
@SethFalco
Copy link
Contributor Author

Actually, just converted this to a draft.
After further testing, this is flawed. 馃

If the query variables editor is modified, so it's malformed, then prettify is clicked, it reverts it back to the last valid input.

@SethFalco
Copy link
Contributor Author

Alright, I believe we should be all good now!
Tests are passing, formatting works, doesn't erase user input, JSON is still being validated, and GraphQL related errors are still displayed on hover!

I'm not too sure on how to use Nunjucks in Insomnia to be honest, everything I do leads to malformed tags. (Even on the stable version.) However, it does format at least. ^-^'

@SethFalco SethFalco marked this pull request as ready for review May 19, 2022 22:11
@SethFalco SethFalco force-pushed the pretty-variables branch 2 times, most recently from 820d2ef to b39c9c0 Compare May 19, 2022 22:14
@chabou
Copy link

chabou commented May 19, 2022

I would love to have variables prettified too! 鉂わ笍

@dimitropoulos dimitropoulos added the insomnia-stream a good candidate to look at during the weekly livestream (see #stream on https://chat.insomnia.rest) label May 20, 2022
@dimitropoulos
Copy link
Contributor

let's look at this on the stream this coming week: come join us on the #stream channel over on the Insomnia community slack: https://chat.insomnia.rest. The next one will be 10:30am ET on Tuesday May 24th.

@SethFalco
Copy link
Contributor Author

SethFalco commented May 24, 2022

Since the stream was cancelled, any chance of getting feedback here, or shall I just wait for next week?

There's no urgency for the PR, but at the same time just want to make sure things are cool if you have any qualms with it. 馃憤馃従

@filfreire
Copy link
Member

Hi @SethFalco, sorry for yesterday's late notice.

We should be able resume the streams and to look at it next week on the next stream 馃檱

@dimitropoulos
Copy link
Contributor

very sorry. it was our first cancellation (I was super sick yesterday with the regular flu (of all things). we're 15/16 weeks, now, so hopefully that means that next week should be all set. If it's ok with you, I'd prefer to wait until the steam on 5/31 just because this kind of thing is so so much easier to work through and explain.

I don't think I have any qualms with it! Just gotta figure out what the best UX should be :)

Right now I absolutely expect this PR to get merged in some form, for what it's worth.

@SethFalco
Copy link
Contributor Author

Absolutely no problem at all! Just wanted to know the plan!
High hopes for next week then. \(^-^)/

@SethFalco
Copy link
Contributor Author

Ahh, as a small heads-up, I can't make it to next week's stream live, so I'll just watch the VOD later to hear what's up!

Copy link
Contributor

@gatzjames gatzjames left a comment

Choose a reason for hiding this comment

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

@SethFalco great work!
I did some testing and found some issues we could look at the stream:

The default value for variables being an empty string breaks new queries
There is a flash of content when the variables editor contains template variables.

@@ -573,10 +586,11 @@ export class GraphQLEditor extends PureComponent<Props, State> {
}

const query = obj.query || '';
const variables = obj.variables || null;
const variables = obj.variables || '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Switching this to empty string results in double quotes "" being set as a default value to variables.
This is not ideal for users since the variables editor expects the user to define a JSON object instead of a string and makes the query invalid.

image

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 should've tested a new query, sorry about that. 馃う馃従
I'll look into it and fix both these issues on Saturday!

@dimitropoulos
Copy link
Contributor

(by the way, here's what I tested with on the stream)

https://rickandmortyapi.com/graphql
query MyQuery($name: String!) {
  characters(page: 2, filter: { name: $name }) {
    info {
      count
    }
    results {
      name
    }
  }
  location(id: 1) {
    id
  }
  episodesByIds(ids: [1, 2]) {
    id
  }
}

query MyQuery2($name: String!) {
  characters(page: 2, filter: { name: $name }) {
    info {
      pages
    }
  }
}
{
  "name": "rick"
}

@SethFalco
Copy link
Contributor Author

SethFalco commented Jun 7, 2022

Thanks for the heads-up! I'll review this and the stream on Friday, if you don't already fix the branch and merge it yourself anyway. (Thanks for looking at it meanwhile!)

Sorry for being a bit slow there, work was doing an on-site last week, so I was away from my PC for a while, and now I'm sick.

@dimitropoulos
Copy link
Contributor

no worries at all! :)

@jackkav
Copy link
Contributor

jackkav commented Oct 27, 2022

Closing, stale.

@jackkav jackkav closed this Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
insomnia-stream a good candidate to look at during the weekly livestream (see #stream on https://chat.insomnia.rest)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants