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.

Sign up for GitHub

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

Jump to bottom

Add new patches to graphql-language-service and remove monaco-editor patch #490

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Add new patches to graphql-language-service and remove monaco-editor patch #490

wants to merge 6 commits into from

Conversation

u9g
Copy link
Contributor

@u9g u9g commented Oct 16, 2023

I remove the monaco-editor patch because it was the wrong fix, and we can just wait for the pr to land as it's just a console message.

This pr adds:

  • autocompleting fields on ctrl+space (on a new line)
  • autocompleting @-directives (after a field name or more @Directives) and will somewhat filter them based on the field's type
  • Only allow interfaces to be autocompleted that implement the type in ... on X fragments
  • Updates pnpm because they retroactively broke our pnpm version for patches as dependency installs fail and the solution update to latest

Future possibilities:

  • Autocomplete op type in @filter?

@u9g u9g changed the title Improve patches on graphql-language-service and remove monaco-editor patch Add new patches to graphql-language-service and remove monaco-editor patch Oct 16, 2023
obi1kenobi
obi1kenobi reviewed Oct 18, 2023
View reviewed changes
Copy link
Owner

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Love the ideas for smarter autocomplete! Thanks for putting this together.

I think I might have found a few remaining edge cases before this is ready to merge and publish. Whenever you're able to take a look, I'll be happy to merge this.

experiments/browser_based_querying/patches/[email protected] Outdated Show resolved Hide resolved
experiments/browser_based_querying/patches/[email protected] Outdated
+ // `state` is `null` when the user autocompletes after typing just '@' then opening autocomplete menu
+ // further restrict the directives we suggest based on the type of the field we are on
+ // x.label does not include the '@'
+ if (primitives.has(typeInfo.fieldDef.type.name)) {
Copy link
Owner

Choose a reason for hiding this comment

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

What happens here if the field type is something like [String!] or [[Int]]!?

Copy link
Contributor Author

@u9g u9g Oct 18, 2023 •
edited

Choose a reason for hiding this comment

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

Amazing point, thank you! Then it will be an object of

{
    "ofType": { // <- the list
        "name": "String" // <- the actual type
    }
}

So the solution is just to iterate, done in 0f15fee

@obi1kenobi obi1kenobi added C-enhancement Category: raise the bar on expectations A-playground Area: the query playground at play.predr.ag labels Oct 18, 2023
@u9g
Fix swapped names
bb3fb60
@u9g
Simplify interface checking
c37b41c 
Don't check over each type's interface's interface's since the parent type must declare all interfaces implemented
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@obi1kenobi obi1kenobi obi1kenobi left review comments

Assignees
No one assigned
Labels
A-playground Area: the query playground at play.predr.ag C-enhancement Category: raise the bar on expectations
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@u9g @obi1kenobi