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

Add support for Apollo Connectors Mapping Syntax #230

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

Conversation

phryneas
Copy link
Member

No description provided.

Copy link
Contributor

github-actions bot commented Oct 18, 2024

You can download the latest build of the extension for this PR here:
vscode-apollo-0.0.0-build-1730376475.pr-230.commit-036c822.zip.

To install the extension, download the file, unzip it and install it in VS Code by selecting "Install from VSIX..." in the Extensions view.

Alternatively, run

code --install-extension vscode-apollo-0.0.0-build-1730376475.pr-230.commit-036c822.vsix --force

from the command line.

For older builds, please see the edit history of this comment.

@phryneas phryneas marked this pull request as ready for review October 28, 2024 14:38
Copy link
Member

@dylan-apollo dylan-apollo left a comment

Choose a reason for hiding this comment

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

I'll be honest, I don't really understand how this works, but your demo looked great! Added a few questions/comments

@@ -0,0 +1,270 @@
# use 'js-yaml' to convert to json
# npx js-yaml syntaxes/connectors.jsonselection.yaml >| syntaxes/connectors.jsonselection.json
name: "Apollo Connectors JSONSelection"
Copy link
Member

Choose a reason for hiding this comment

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

Is this user-facing at all? We've avoided using this name anywhere user-facing for now, since we're going to change it, we just haven't decided to what yet 😅

Copy link
Member Author

@phryneas phryneas Oct 29, 2024

Choose a reason for hiding this comment

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

That's a good call-out - the only "user-facing" thing right now is that we introdude a new language to VSCode:

vscode-graphql/package.json

Lines 159 to 165 in 0ea12de

{
"id": "apollo.jsonselection",
"filenames": [
"test.apollo.jsonselection"
],
"configuration": "./apollo.jsonselection.configuration.json"
},

That means it will pop up if you edit a file called test.apollo.jsonselection, but also if you use the "change file language" dropdown:

image

We might want to give that language a better user-facing name.

It's technically also visible if you inspect syntax highlighting tokens, but I would not really consider that "user-facing":

image

What should I better name this instead? apollo.connectors.syntax?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that sounds generic enough to be okay. We also use the term "mapping" a lot when referring to it right now, like "Apollo Connectors Mapping Syntax", though that's really verbose.

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Renamed it, and also put it into [] because that will sort it alphabetically at the bottom of the list. There's no need for this implementation detail to be the first thing on the list every time people open it :)

Comment on lines +42 to +43
"1":
name: "string.unquoted.NamedPathSelection.alias"
Copy link
Member

Choose a reason for hiding this comment

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

So can each of these sections get their own colors? I'd love to have one color for the "output" name (alias in this case) and another for things that are inputs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, they can!

Open a text editor and execute the "Developer: inspect editor tokens and scopes" command

image

that will give you this pop-up:

image

And then you can move your cursor around and see what tokens things in other languages have, as an inspiration.


image
image

You see that GraphQL uses string.unquoted and variable as highlighting scopes here, but I also see that the alias is never applied because variable.KeyPath.key.identifier seems to have a higher priority somehow.

I'll debug that :)

Copy link
Member Author

Choose a reason for hiding this comment

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

So... with some changes:

image

Copy link
Member

Choose a reason for hiding this comment

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

Amazing! A couple more notes on what we might want to add in future iterations:

  1. $ and @ as well as an identifier following $, like $args, $this, etc. are sort of our keywords? They're special, at very least, and might be worth highlighting separately.
  2. Anything in $() or as an argument to a method is sort of a different language mode. We call them literals, but they can also have some expressions within them. So ${ blah: $.blah } and $({ blah: $.blah }) are both valid, and look very similar, but do different things. It'd be nice to highlight that difference somehow (effectively, just the presence of the parens causes it).

Copy link
Member Author

Choose a reason for hiding this comment

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

I can configure {( and )} as a special kind of bracket pairs (this is VSCode, not TextMate!) so they will always have the same color and highlight in pairs together:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

@lennyburdette then I'm undoing that change and we go back to this:
image

Honestly I don't know if I can do a lot to visually highlight that difference more, apart from adding a full other "sub-language", which I'd rather avoid as this would become even more unmaintainable 😅

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we just color parens differently than {} for now, since parens always represent entering literal mode? We could do something special with semantic tokens or something if we really want in the future

Copy link
Member

Choose a reason for hiding this comment

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

The @ and $ stuff looks great!

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we just color parens differently than {} for now, since parens always represent entering literal mode? We could do something special with semantic tokens or something if we really want in the future

We'll need to test that out in the non-VSCode editors. VSCode pretty much ignores everything else and applies its "bracket pair coloring" logic over the top of configured bracket pairs (which we want to have so it highlights start and end).

Copy link
Member

Choose a reason for hiding this comment

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

Then maybe it's just fine 😅. Certainly shouldn't block getting the highlighting of the rest of the language out there

syntaxes/graphql.connectors.json Outdated Show resolved Hide resolved
syntaxes/graphql.connectors.json Outdated Show resolved Hide resolved
@phryneas phryneas changed the title experiment: Textmate connectors highlighting Add support for Apollo Connectors Mapping Syntax Oct 31, 2024
Comment on lines +345 to +352
"contentName": "meta.embedded.block.directive.graphql",
"begin": "\\s*(?=@)",
"end": "(?=.)",
"applyEndPatternLast": 1,
"patterns": [{ "include": "#graphql-directive-body" }]
},
"graphql-directive-body": {
"begin": "((@)\\s*([_A-Za-z][_0-9A-Za-z]*))",
Copy link
Member Author

Choose a reason for hiding this comment

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

I managed to get this down to a minimum amount of changes, but unfortunately not down to none.

Comment on lines +1 to +7
scopeName: source.apollo.connectors.graphql
# inject these rules into source.graphql#graphql-directive, before all other patterns (:L), ignoring strings and comments
injectionSelector: L:meta.embedded.block.directive.graphql -string -comment
patterns:
- include: "#connect-directive"
repository:
# if we encounter a `@connect` directive, move forward into this set of rules, otherwise it will just follow the default graphql rules
Copy link
Member Author

Choose a reason for hiding this comment

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

I added a good bunch of comments to this file, please let me know if they are helpful or if I could make something more clear here.

"debug.node.autoAttach": "on"
"debug.node.autoAttach": "on",
"yaml.schemas": {
"https://raw.githubusercontent.com/martinring/tmlanguage/master/tmlanguage.json": "/syntaxes/*.yaml"
Copy link
Member Author

Choose a reason for hiding this comment

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

I found this JSONSchema which somehow adds a tiny little bit of documentation to the textmate format.

Comment on lines +416 to +420
> `query Q11 { test }`
#^^ source.ts
# ^ source.ts string.template.ts punctuation.definition.string.template.begin.ts
# ^^^^^^^^^^^^^^ source.ts string.template.ts
# ^ source.ts string.template.ts punctuation.definition.string.template.end.ts
# ^^^^^^^^^^^^^^^^^^ source.ts string.template.ts
# ^ source.ts string.template.ts punctuation.definition.string.template.end.ts
Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated, it seems I haven't updated some test snapshots in a while.

@phryneas
Copy link
Member Author

I think this is in a good state right now, so I'll wait a bit for feedback from testing before we can hopefully get this in next week or so :)

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.

3 participants