feat(graphql-config): add graphql config extensions #1118

divyenduz · 2019-12-17T20:56:12Z

This PR is a port of graphql/graphql-language-service#240

It adds GraphQL config extension support to the language server.

This would enable the following scenarios (not-exhaustive):

Using environment variables (or variables in a custom format)
Supply customDirectives dynamically - can be tool specific like
Gatsby
Supply customValidationRules dynamically
Add endpoints, headers dynamically

This PR is a port of graphql/graphql-language-service#240 It adds GraphQL config extension support to the language server. This would enable the following scenarios (not-exhaustive): 1. Using environment variables (or variables in a custom format) 2. Supply customDirectives dynamically - can be tool specific like Gatsby 3. Supply customValidationRules dynamically 4. Add endpoints, headers dynamically

acao · 2019-12-17T21:00:16Z

packages/graphql-language-service-server/src/MessageProcessor.js

+    let config = getGraphQLConfig(rootPath);
+    if (this._extensions && this._extensions.length > 0) {
+      /* eslint-disable no-await-in-loop */
+      for (let i = 0; i < this._extensions.length; i++) {


we have support for the latest language features now, so if you want to use for... await of here you can! I think Its node 9+.

Bumped the loop to new syntax but kept /* eslint-disable no-await-in-loop */ becuase lint still disables it.

@divyenduz interesting note, perhaps we should change this rule now that we have modern for...await of

acao · 2019-12-17T21:05:28Z

this looks great @divyenduz, thanks!

codecov · 2019-12-17T21:06:42Z

Codecov Report

❗ No coverage uploaded for pull request base (master@b0edf6e). Click here to learn what that means.
The diff coverage is 42.85%.

@@            Coverage Diff            @@
##             master    #1118   +/-   ##
=========================================
  Coverage          ?   43.98%           
=========================================
  Files             ?       64           
  Lines             ?     3008           
  Branches          ?      652           
=========================================
  Hits              ?     1323           
  Misses            ?     1414           
  Partials          ?      271

Impacted Files	Coverage Δ
...graphql-language-service-server/src/startServer.js	`0% <0%> (ø)`
...raphql-language-service-server/src/GraphQLCache.js	`54.93% <50%> (ø)`
...ql-language-service-server/src/MessageProcessor.js	`44.13% <66.66%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b0edf6e...9e3e973. Read the comment docs.

acao · 2019-12-17T21:13:43Z

oh dear, looks like codecov blocked it. but yeah this is right up our alley in terms of our intended route with graphql config. I’m looking forward to getting the LanguageService class and its own GraphQLCache working in the browser soon. We may end up figuring out how graphql config could be one config service, or you can bring your own, but that seems too complicated. different platforms want to have their own config formats, but maybe we can introduce something to graphql-config where they could specify different extensions and map their values to graphql-config values?

divyenduz · 2019-12-17T21:21:35Z

but maybe we can introduce something to graphql-config where they could specify different extensions and map their values to graphql-config values?

That is interesting indeed, are you suggesting that we move this feature from here into the GraphQL config? IIRC they had some extension work done in GraphQL Config back then, we chose to do this feature back then in LSP because we wanted dynamic config changes (my memory on this is faded, but we can surely re-think it)

Urigo · 2019-12-17T21:52:27Z

@kamilkisiela has already introduced a start for extending GraphQL Config for different tools in the new alpha.

@divyenduz we would love to chat and see if that fits your use case or should we change anything!

acao · 2019-12-20T12:51:59Z

@divyenduz just needs some more test coverage to get builds passing! maybe we can get this merged before the typescript conversion is merged?

divyenduz · 2019-12-20T13:54:02Z

@acao When do you plan to merge your refactor? I am traveling over the weekend. I can maybe get to this on 26th? (or will see if I can find a moment from an airport or something 😅)

acao · 2019-12-20T16:41:39Z

it’s almost finished, just two more tests, but then itll need plenty of time to review. that should be plenty of time! enjoy your holidays, thank you @divyenduz !

acao · 2020-01-18T13:45:32Z

@divyenduz this one should be relatively easy to update for typescript when you get a chance? can you update docs as well?

divyenduz · 2020-01-18T13:47:30Z

@acao Is this even valid after #1225 is merged? I need to build more context on that but it looks like they are removing the extensions feature, making this PR already outdated 😅

acao · 2020-01-18T14:07:37Z

@divyenduz I think they are deprecating the endpoints extension specifically, however I think extensions will remain!

https://graphql-config.com/docs/introduction

so this feature is very useful.

@ardatan @kamilkisiela agreed this is still compatible with 3.0.0-alpha-n? on top of the graphql-config migration PR @ardatan already created?

acao · 2020-01-18T15:02:41Z

@divyenduz an alpha release is now available :D

divyenduz · 2020-03-12T22:40:24Z

packages/graphql-language-service-server/src/startServer.ts

@@ -46,10 +47,13 @@ type Options = {
  // port for the LSP server to run on
  port?: number;
  // socket, streams, or node (ipc). if socket, port is required
-  method?: 'socket' | 'stream' | 'node';


This looks incorrect. I think this should be 'socket' | 'stream' | 'node'

Fixed via f6d8a56

divyenduz · 2020-03-12T22:42:15Z

@acao I wonder why CI tests that blocked me last time didn't appear this time? 🤔

acao · 2020-03-12T23:47:01Z

@divyenduz that is strange, i wonder if there's an issue with github actions?

acao · 2020-03-12T23:49:17Z

possibly because this PR pre-dates the switch to github actions?
but that wasnt an issue when we first switched?
was it a travis failure or gh actions failure before?

divyenduz · 2020-03-13T15:38:55Z

@acao haha, weird indeed, I was talking about this comment: #1118 (comment)

Maybe GH actions don't run for older PRs. I can create a new PR and replace this one with that. Does that sounds good?

acao · 2020-03-13T20:56:40Z

oh right! i have not restored the codecov behavior in github actions, my bad, forgot to do that! yikes.

divyenduz · 2020-03-14T03:21:41Z

But still, even the tests did not run! 🤔

I see that they are running for other PRs. I will just re-create this one to see if it runs.

divyenduz · 2020-03-14T03:34:46Z

"Workflows in forked repositories don't run by default." via https://help.github.com/en/actions/getting-started-with-github-actions/about-github-actions

I couldn't find the docs to enable it though, looking for it.

✅ Green in my fork: https://github.com/divyenduz/graphiql/runs/507245373?check_suite_focus=true

Haha! Let me know how to proceed, we can

Merge this one and clean it up later! (by adding codecov + tests)
Clean it up now

I would prefer 1 for now, just to get this PR out of the door, it has been looooong living 😄

Update: I think this is how we enable it on forked repos: https://help.github.com/en/actions/reference/events-that-trigger-workflows#pull-request-events-for-forked-repositories

By adding pull-request as an event to the workflow.

But it looks like this doesn't work: https://github.community/t5/GitHub-Actions/Github-Workflow-not-running-from-pull-request-from-forked/td-p/32979

Quickly trying it out in this branch.

divyenduz · 2020-03-14T03:42:53Z

Yes it works! Precioussssss! <3

divyenduz · 2020-03-14T04:16:10Z

packages/graphql-language-service-server/src/MessageProcessor.ts

+    if (this._extensions && this._extensions.length > 0) {
+      /* eslint-disable no-await-in-loop */
+      for (const extension of this._extensions) {
+        config = await extension(config);
+      }
+    }


I don't think that this is required here since we do the same thing in the getGraphQLCache function.

Removed it, nice, this was a code smell with duplicate code. Moving config to cache solves this 👍

acao · 2020-03-14T04:28:50Z

nice work! thanks @divyenduz

acao · 2020-03-14T04:32:06Z

packages/graphql-language-service-server/src/GraphQLCache.ts

+  if (extensions && extensions.length > 0) {
+    /* eslint-disable no-await-in-loop */
+    for (const extension of extensions) {
+      graphQLConfig = await extension(graphQLConfig);


@divyenduz so this is intended to re-declare graphQLConfig on every invocation? I'm guessing the result of await extension(graphQLConfig) returns the previous config plus the extension?

Yes, as of now, the signature of an extension is a function that takes a GraphQLConfig as input and returns a GraphQL config as its output with the modification (extension) applied.

extensions?: Array<(config: GraphQLConfig) => GraphQLConfig>
Which implies, extension: (config: GraphQLConfig) => GraphQLConfig

acao · 2020-03-14T04:33:10Z

maybe we can update docs, as well?

acao · 2020-03-14T04:48:43Z

https://deploy-preview-1118--graphiql-test.netlify.com/lsp/modules/graphql_language_service_server.html#getgraphqlcache fyi, interesting how tsdoc handles this as Array<function>. I think I need to tweak the tsdoc configuration (another matter)

divyenduz · 2020-03-14T04:51:48Z

Added a test, figuring out where the docs live!

Does your tsdoc comment mean that we get docs for free (after we fix the array function representation?) or do I need to do something else as well? :)

acao · 2020-03-14T04:55:42Z

@divyenduz good question! let’s put the docs for this in graphql-language-service for now. there will be a doc site soon.

the lsp api docs are nice but not instructive enough yet, or fully configured, and the readme’s come with

divyenduz · 2020-03-14T05:09:08Z

@acao Added some docs to the README of graphql-langauge-service package. Please see if that makes sense :)

divyenduz · 2020-03-14T15:14:38Z

@acao I did a split of CLI and Server docs as discussed. There is some duplication in the two readme files, please let me know if any further changes are needed :)

acao · 2020-03-14T16:29:39Z

@divyenduz looking great! we are ready to merge then

acao reviewed Dec 17, 2019

View reviewed changes

fix(extensions): use new node syntax

1dd5c00

ardatan mentioned this pull request Jan 14, 2020

WIP: Use new GraphQL Config #1225

Closed

2 tasks

Merge branch 'master' into feat-extensions

f4d926e

acao mentioned this pull request Feb 16, 2020

feat: use new GraphQL Config #1342

Merged

2 tasks

acao added this to In progress in 3. Complete LSP Features via automation Mar 10, 2020

chore: merge with master

5cdc15e

divyenduz commented Mar 12, 2020

View reviewed changes

fix: type of method in startServer

f6d8a56

divyenduz mentioned this pull request Mar 14, 2020

Feat extensions recreated #1423

Closed

chore: add pull_request to ci trigger

4d9138d

divyenduz commented Mar 14, 2020

View reviewed changes

refactor: remove duplicate extension apply code

1e43e06

acao reviewed Mar 14, 2020

View reviewed changes

test: add test for getGraphQLCache

2fe3e1d

docs: add docs for startServer function

f0f5af2

Divyendu Singh added 2 commits March 14, 2020 10:47

chore: remove stale disabled deslint rule

45c9323

docs: split readme of cli, server

9e3e973

acao changed the title ~~feat(extensions): add graphql config extensions~~ feat(graphql-config): add graphql config extensions Mar 14, 2020

acao merged commit 2a77e47 into graphql:master Mar 14, 2020

3. Complete LSP Features automation moved this from In progress to Done Mar 14, 2020

divyenduz deleted the feat-extensions branch October 21, 2022 14:13

feat(graphql-config): add graphql config extensions #1118

feat(graphql-config): add graphql config extensions #1118

Conversation

divyenduz commented Dec 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acao commented Dec 17, 2019

codecov bot commented Dec 17, 2019 • edited Loading

Codecov Report

acao commented Dec 17, 2019

divyenduz commented Dec 17, 2019

Urigo commented Dec 17, 2019

acao commented Dec 20, 2019 • edited Loading

divyenduz commented Dec 20, 2019

acao commented Dec 20, 2019

acao commented Jan 18, 2020 • edited Loading

divyenduz commented Jan 18, 2020 • edited Loading

acao commented Jan 18, 2020 • edited Loading

acao commented Jan 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divyenduz commented Mar 12, 2020

acao commented Mar 12, 2020

acao commented Mar 12, 2020

divyenduz commented Mar 13, 2020

acao commented Mar 13, 2020

divyenduz commented Mar 14, 2020

divyenduz commented Mar 14, 2020 • edited Loading

divyenduz commented Mar 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acao commented Mar 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acao commented Mar 14, 2020

acao commented Mar 14, 2020 • edited Loading

divyenduz commented Mar 14, 2020

acao commented Mar 14, 2020 • edited Loading

divyenduz commented Mar 14, 2020

divyenduz commented Mar 14, 2020

acao commented Mar 14, 2020

codecov bot commented Dec 17, 2019 •

edited

Loading

acao commented Dec 20, 2019 •

edited

Loading

acao commented Jan 18, 2020 •

edited

Loading

divyenduz commented Jan 18, 2020 •

edited

Loading

acao commented Jan 18, 2020 •

edited

Loading

divyenduz commented Mar 14, 2020 •

edited

Loading

acao commented Mar 14, 2020 •

edited

Loading

acao commented Mar 14, 2020 •

edited

Loading