Add client schema support to vscode extension

[jasonpaulos]

Specifically,

• Augment the client schema with metadata about which fields/objects
  are client-only.
• Show information about client-only fields on hover.
• Add auto complete support for injecting client directives when
  appropriate.
• Add a validation rule to detect missing client directives.
• Add a code action to suggest the addition of a clienet directive
  if it is missing. Also add the ability in general for validation
  rules to do this.
• Add go to definition support for custom client-only directives.
• Add information on hover for directives.
• Add auto complete support for directives.
• Forbid unknown directives.

---

Since this update forbids unknown directives from being used, libraries like apollo-link-rest that provide their own client directives used for links will have to make the extension aware of their directives. This can be done like so (assuming the package comes with a schema.graphql file [which none do currently]):

var apolloRestSchema = require.resolve('apollo-link-rest/schema.graphql');

module.exports = {
  client: {
    name: 'Space Explorer [web]',
    service: 'apollographql-8341',
    includes: [
      'src/**/*.{ts,tsx,js,jsx,graphql,gql}',
      apolloRestSchema
    ]
  },
};

To make it easier to use the vscode extension with apollo-client, the extension has a baked-in copy of its directive definitions which are included if it detects that none of the apollo-client directives are already defined (either from Relay's @connection or someone including the apollo-client schema like above). This means that no config changes should be needed for developers who are only using apollo-client's directives.

---

Fixes #847.

---

Progress:

• Initial features 🎉
• Decide if including the KnownDirectivesRule validation rule causes more good than harm.
  • I definitely want to include this. Everyone should be taking advantage of "build time" validation.
• Think about how plugins like apollo-link-rest can add custom client directives (should we bake them into the extension or should they live in their own plugin?)
  • 🙅‍♂ No to baked in directives! Client schema extensions need to be configurable in the apollo.config.js config file.
• Figure out if I can use permalinks to our docs instead of the current linking in the baked in descriptions of client directives.
  • Permalinks are not available at the moment
• Get tests passing
• Add to changelog

[alloy]

This is looking great! 🙌

From a Relay perspective, it would be great if the default client directives could entirely be overridden and ideally by assigning the SDL (as a string) to the project config–which would make it possible to create a vscode-apollo-relay package which would know how to fetch the SDL for Relay’s directives from the Relay packages. Currently my vscode looks like this:

[alloy]

Just did a bit more testing to see if I could at least define all of Relay’s directives by adding a .graphql file to my source tree. This works for those that are not already defined in your PR. E.g. if I add the definition for the relay directive it is removed from the problems pane:

But then if I add the definition of Relay’s @connection directive it seems to also undo the definition of the @relay directive:

[jasonpaulos]

Great feedback @alloy! It's clear to me now that hardcoded definitions of Apollo directives may not be the best choice.

I've made some changes so that the Apollo directives are no longer included in the base extension; rather, the extension now includes as a default search path node_modules/*/schema.graphql, so that individual modules that expose custom client-side directives (like apollo-client and apollo-link-rest) can define them in a schema.graphql file that will automatically be used for validation, autocomplete, hover information, etc. by the extension.

The advantages of this approach are:

• The directive definitions are controlled in the same codebase as their implementation, so the vscode extension will never contain out of date definitions.
• Even though a schema.graphql will have to be added to frontend libraries like apollo-client, it should get ignored when bundling and won't add to the bundle size.
• Since the directives are defined in actual files, VSCode will let you cmd+click (or equivalent) to go to their definitions.
• It allows us to show an error for unknown directives while still exposing an easy way for 3rd party libraries/frameworks to included custom directives.

And of course, if you override the includes option and/or provide your own local schema like in your screenshot @alloy, you can add Relay definitions easily without them conflicting with Apollo definitions.

I'm interested to hear thoughts about this approach and any potential downsides!

Edit: looks like some tests are failing because apollo-client does not yet provide a schema.graphql file with the @client directive, so the extension is throwing an error when it encounters the unknown directive! This will be super useful to frontend devs whenever they misspell or use a directive incorrectly.

[jasonpaulos]

After discussing with @jbaxleyiii, we concluded that having the extension automatically check for node_modules/*/schema.graphql files and include them in the client schema has some faults. For projects with many dependencies, it can be very expensive to search through all of node_modules, which would impact the extension's startup time. Additionally, it's possible that some dependencies of dependencies might contain schema declarations that are unwanted by developers.

Since it's still very important to allow developers and package authors to configure, override, and include custom client schema like directives, I am currently figuring out a way we can support these processes in the apollo.config.js file. I believe the config file is a more natural place for this to happen, as long as the options and common use cases are documented. I will include commits/details here after I have more concrete ideas for what this could look like.

[alloy]

Sorry for the delay in replying, but moving it to the config file echoes my thoughts entirely 👍

[trevor-scheer]

Just some non-blocking nitpicks, this LGTM!

[alloy]

(@jasonpaulos Let me know when I should give it another go.)

[jasonpaulos]

@alloy Feel free to review this again! Now the extension will add in Apollo directives like @client, @export, etc. by default, unless a directive by the same name already exists in an included file. So since Relay defines @connection already, you shouldn't see any Apollo directives being suggested in typeahead. Let me know if this works for you.

[JakeDawkins]

I'm confused why this isn't being built by azure 🤔

[JakeDawkins]

@jasonpaulos checked this on windows, and everything seems to be working properly :)

[JakeDawkins]

@alloy any update here? Have you gotten a chance to give it another pass?

[alloy]

I have not. I’ll try to do so later today, but as I’m on holiday I may not get to it and you should just merge.

Reading @jasonpaulos’ description it seems like Apollo specific directives may still be auto-completed/recognized in schemas that don’t support those, unless they happen to provide their own copy, is that correct?

[alloy]

I had a bit of a problem running the vscode extension directly from the repo (missing apollo-env), so I ended up testing after running npm run-script package-extension. Let me know if that was an unreliable way to test.

• I think, if I read the source right, that none of the apollo directives get defined if any directive clashes, which seems to work for relay 👍 It still feels a bit odd to me that in other situations the directives do end up getting defined perhaps wrongly–is there no good heuristic for the extension to know when somebody is using apollo server?

• Was there a 'no unknown directives' validation rule previously, or am I imagining that? Currently I'm able to write any directive in any place and no problems are reported.

• It is not clear to me how I can provide the directive definitions programatically via the config file, other than specifying a .graphql file that contains them.

[jasonpaulos]

@alloy thanks for taking the time to review! You are correct that none of the Apollo directives will be defined if there's any name collisions with already defined directives. We want to make the extension as useful as possible for Apollo users, so it made sense for us to always try to include those directives. Perhaps we could introduce a config option to explicitly disable them, like includeApolloClientDirectives: false if there is a significant use case for it.

Also, by default the extension does now include the graphql KnownDirectivesRule, so it's strange that you were not seeing validation errors with unknown directives. Perhaps you were overriding the validation rules in the config? Or maybe the missing apollo-env build problem caused weird behavior--if you run npm run clean && npm install this should fix that problem.

Lastly, the only way to add in extra definitions is by including them in your config like below, which I think is pretty reasonable.

var apolloRestSchema = require.resolve('apollo-link-rest/schema.graphql'); // or some other way to get to a file containing SDL

module.exports = {
  client: {
    name: 'Space Explorer [web]',
    service: 'apollographql-8341',
    includes: [
      'src/**/*.{ts,tsx,js,jsx,graphql,gql}',
      apolloRestSchema
    ]
  },
};

[alloy]

Also, by default the extension does now include the graphql KnownDirectivesRule, so it's strange that you were not seeing validation errors with unknown directives. […
Or maybe the missing apollo-env build problem caused weird behavior--if you run npm run clean && npm install this should fix that problem.

Had to reclone in order to get a working build again, but after that indeed the KnownDirectivesRule showed up and I was getting problems reported 👌

Lastly, the only way to add in extra definitions is by including them in your config like below, which I think is pretty reasonable.

It’s definitely not unreasonable 😄 I’m unsure what other frameworks might do, but Relay doesn’t define all of its directives in graphql files, but rather as strings inside JS modules. For example, here’s the @relay directive. So other than collecting them all, concatenating them into a temp file, and feeding that file to the config, as per your example, I don’t see a way to refer to those.

I’m fine going with that as a first version and see how it pans out, though, if you think having a way of specifying such schema extensions as a string would be tough 👍

---

I just now realised another tougher issue in combination with Relay, which is its @argumentDefinitions and @arguments directives, which define (pseudo) local arguments to fragments.

The problem with these directives is that their parameters are by definition unknown upfront, which is probably also why relay-compiler handles these directly in its parser. Seeing as there is no way to have an argument signature that says “accept anything”, usage of these directives leads to problems being reported:

I don’t see a solution other than disabling the KnownArgumentNames rule entirely, which is a shame. If you see an opportunity for how this could be improved upon, then please do let me know 🙏

Having said all that, this all is in no way a blocker for this PR imo, which is looking like a great next step forward for this vscode extension 👏

[alloy]

(I think the @argumentDefinitions and @arguments directives can only really be handled well by having code running in the extension that understands them, which would also allow for discovering and validating the correct arguments are passed to the right fragments.)

[alloy]

Following up on my earlier comments, I was able to make it all work. E.g. here’s a KnownArgumentNames validation rule that knows how to handle Relay’s fragment arguments: https://github.com/relay-tools/vscode-apollo-relay/blob/master/src/RelayKnownArgumentNames.ts

[cltnschlosser]

Hey guys, we've found this commit to be the cause of a huge performance regression in codegen. See #1554 (comment) for more details, hopefully we can improve this approach.

[pirtlj]

Any thought about putting this back in at some point?