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: Field alias completions #4135

Conversation

beaumontjonathan
Copy link
Contributor

@beaumontjonathan beaumontjonathan commented Nov 22, 2022

Fix field alias completions

The parser currently stops parsing documents when it encounters incomplete aliases, e.g.,

query {
  user {
    aliasField: # Expected a non-variable identifier (e.g. 'x' or 'Foo')
  }
}

It reports that it's expecting an identifier, which is the correct error, but reports it on the span of the next token.

Changes:

  • stops the parser from exiting early by placing reporting an error then placing an empty identifier in the tree
  • adds a more verbose error message
  • refactor: added new helper fn empty_identifier(&self) -> Identifier on Parser because now 3 places are making use of this "empty identifier" & record error pattern

Before

Screenshot 2022-11-22 at 23 00 50

Before (error message visible)

Screenshot 2022-11-22 at 23 00 12

After

Screenshot 2022-11-22 at 22 57 56

Now that parsing continues, multiple errors can be reported whereas previously the parser would bail at the first invalid alias

Screenshot 2022-11-23 at 10 15 10

Copy link
Contributor

@alunyov alunyov left a comment

Choose a reason for hiding this comment

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

Thank you for adding this change!

Can you add a test with the updated error message?

@beaumontjonathan
Copy link
Contributor Author

@alunyov Fair comment, I've added a test 😄

@facebook-github-bot
Copy link
Contributor

@alunyov has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

zth pushed a commit to zth/relay that referenced this pull request Nov 30, 2022
Summary:
# Fix field alias completions

The parser currently stops parsing documents when it encounters incomplete aliases, e.g.,

```graphql
query {
  user {
    aliasField: # Expected a non-variable identifier (e.g. 'x' or 'Foo')
  }
}
```

It reports that it's expecting an identifier, which is the correct error, but reports it on the span of the next token.

## Changes:
- stops the parser from exiting early by placing reporting an error then placing an empty identifier in the tree
- adds a more verbose error message
- refactor: added new helper `fn empty_identifier(&self) -> Identifier` on `Parser` because now 3 places are making use of this "empty identifier" & record error pattern

## Before

<img width="912" alt="Screenshot 2022-11-22 at 23 00 50" src="https://user-images.githubusercontent.com/17055343/203438640-e1e522f5-6d35-4682-9a6b-6d4e6487593d.png">

#### Before (error message visible)

<img width="912" alt="Screenshot 2022-11-22 at 23 00 12" src="https://user-images.githubusercontent.com/17055343/203438644-88bf0339-3d26-4c0a-9f3b-f41e4d5568a0.png">

## After

<img width="912" alt="Screenshot 2022-11-22 at 22 57 56" src="https://user-images.githubusercontent.com/17055343/203438647-e4558b3a-9a88-457d-bcd7-47f53c96f65b.png">

Now that parsing continues, multiple errors can be reported whereas previously the parser would bail at the first invalid alias

<img width="949" alt="Screenshot 2022-11-23 at 10 15 10" src="https://user-images.githubusercontent.com/17055343/203521931-e6bb9adc-2b20-4a16-bc8b-08ef8f013799.png">

Pull Request resolved: facebook#4135

Reviewed By: tyao1

Differential Revision: D41505486

Pulled By: alunyov

fbshipit-source-id: 0f776badd05819b189b6d945785b86754db26418
zth pushed a commit to zth/relay that referenced this pull request Nov 30, 2022
Summary:
# Fix field alias completions

The parser currently stops parsing documents when it encounters incomplete aliases, e.g.,

```graphql
query {
  user {
    aliasField: # Expected a non-variable identifier (e.g. 'x' or 'Foo')
  }
}
```

It reports that it's expecting an identifier, which is the correct error, but reports it on the span of the next token.

## Changes:
- stops the parser from exiting early by placing reporting an error then placing an empty identifier in the tree
- adds a more verbose error message
- refactor: added new helper `fn empty_identifier(&self) -> Identifier` on `Parser` because now 3 places are making use of this "empty identifier" & record error pattern

## Before

<img width="912" alt="Screenshot 2022-11-22 at 23 00 50" src="https://user-images.githubusercontent.com/17055343/203438640-e1e522f5-6d35-4682-9a6b-6d4e6487593d.png">

#### Before (error message visible)

<img width="912" alt="Screenshot 2022-11-22 at 23 00 12" src="https://user-images.githubusercontent.com/17055343/203438644-88bf0339-3d26-4c0a-9f3b-f41e4d5568a0.png">

## After

<img width="912" alt="Screenshot 2022-11-22 at 22 57 56" src="https://user-images.githubusercontent.com/17055343/203438647-e4558b3a-9a88-457d-bcd7-47f53c96f65b.png">

Now that parsing continues, multiple errors can be reported whereas previously the parser would bail at the first invalid alias

<img width="949" alt="Screenshot 2022-11-23 at 10 15 10" src="https://user-images.githubusercontent.com/17055343/203521931-e6bb9adc-2b20-4a16-bc8b-08ef8f013799.png">

Pull Request resolved: facebook#4135

Reviewed By: tyao1

Differential Revision: D41505486

Pulled By: alunyov

fbshipit-source-id: 0f776badd05819b189b6d945785b86754db26418
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants