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

Prevent GraphQL non breaking space #2770

Merged
merged 6 commits into from
Sep 1, 2021

Conversation

thewheat
Copy link
Contributor

From https://github.com/Kong/insomnia/pull/2704/files#r505491859 it was found that the code to prevent GraphQL non breaking spaces (that cause a syntax error) is not working

This PR fixes the behaviour

Demo

  • Bottom Insomnia is latest version with the bug
  • Top Insomnia is the version with my PR code
  • Demo copies invalid text and shows it fails in the latest version but passes with the code change

graphql-nbsp-fixed

@@ -804,16 +804,16 @@ class CodeEditor extends React.Component {
change.update(change.from, change.to, [text]);
}

// Don't allow non-breaking spaces because they break the GraphQL syntax
if (doc.options.mode === 'graphql' && change.text && change.text.length > 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

change.text is an array of the text that is inputted with each element being a line of text that was entered

  • most cases would just be a single line so change.text.length == 1
  • if Enter is pressed or text with newline is pasted then change.text.length will be greater than 1

@CLAassistant
Copy link

CLAassistant commented Dec 6, 2020

CLA assistant check
All committers have signed the CLA.

@gatzjames gatzjames self-assigned this Aug 30, 2021
@vercel vercel bot temporarily deployed to Preview September 1, 2021 14:49 Inactive
@gatzjames
Copy link
Contributor

Merged with latest develop and added more irregular characters that might break the interface.

graphql-whitespace-gif

@vercel vercel bot temporarily deployed to Preview September 1, 2021 19:54 Inactive
Copy link
Contributor

@dimitropoulos dimitropoulos left a comment

Choose a reason for hiding this comment

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

looked good. I enjoyed learning of this rick and morty api along the way, haha.

image

@dimitropoulos dimitropoulos merged commit 6ca2bf7 into Kong:develop Sep 1, 2021
@@ -990,7 +991,7 @@ class CodeEditor extends Component<Props, State> {
}
}

_codemirrorValueBeforeChange(doc, change) {
_codemirrorValueBeforeChange(doc: CodeMirror.Editor, change: CodeMirror.EditorChangeCancellable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@gatzjames I believe we should be using CodeMirror.EditorFromTextArea instead of the generic CodeMirror.Editor; that's the only type of editor used AFAIK

if (doc.getOption('mode') === 'graphql') {
const text = change.text.map(normalizeIrregularWhitespace);

change.update?.(change.from, change.to, text);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we shouldn't invoke update if text.length === 0 (ie. no lines)?

@@ -0,0 +1,33 @@
// List of irregular whitespace characters adopted from https://eslint.org/docs/rules/no-irregular-whitespace#rule-details
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice fix here 🙂

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.

6 participants