Switch to graphql-config #130

RomanHotsiy · 2017-07-20T12:50:58Z

hey @asiandrummer ,
here is the PR to switch from graphql-language-services-config to graphql-config.
I split it into 2 commits. First one removes graphql-language-services-config folder and the second one adds support for graphqlconfig

All the tests are passing and flow doesn't show any errors. Although it definitely requires looking from someone who knows the codebase

asiandrummer · 2017-07-21T03:18:59Z

Checking this out. Thanks!

asiandrummer · 2017-07-21T09:41:56Z

@RomanGotsiy This looks amazing! I think I'll be able to merge this, but just in case am doing a super granular read for safety. One high level feedback - would it be possible to PR out the removal of graphql-language-service-config separately?

RomanHotsiy · 2017-07-21T09:50:50Z

@asiandrummer

I think I'll be able to merge this, but just in case am doing a super granular read for safety

Definitely! Would be great if you also can test it with Nuclide if possible as there may be some issues in graphql-config.

would it be possible to PR out the removal of graphql-language-service-config separately?

I think that no. Other packages depend on graphql-language-service-config so tests won't pass and flow will complain.

One more thing: there is probably a need to add some function for detecting and converting old config format to new just to support old configs. Do you think this is required? Do you want me to add this?

asiandrummer · 2017-07-21T10:29:27Z

One more thing: there is probably a need to add some function for detecting and converting old config format to new just to support old configs. Do you think this is required? Do you want me to add this?

I just realized that you've changed includeDirs and excludeDirs to include and exclude. I probably missed this while checking out the PR from graphql-config repo - is it too late to revert back to includeDirs and excludeDirs? I think include and exclude might be too generic to specify what it wants to accomplish.

Since I'm late to suggest this change, if you've already made a lot of PRs to apply this to other repos, I'm fine with writing a function to enable backward-compatibility.

asiandrummer · 2017-07-21T10:40:41Z

packages/graphql-language-service-interface/src/GraphQLLanguageService.js

-        this._graphQLConfig.getSchemaPath(appName),
-        appName,
-      );
+    if (projectConfig.schemaPath) {


I like this getter stuff ;)

asiandrummer

Requesting changes for getAutocompleteSuggestions and the performance optimization, or we can discuss about the perf stuff.

asiandrummer · 2017-07-21T10:42:15Z

packages/graphql-language-service-interface/src/GraphQLLanguageService.js

@@ -103,10 +98,7 @@ export class GraphQLLanguageService {
    const appName = this._graphQLConfig.getAppConfigNameByFilePath(filePath);


Missed here I think

Needs to be:

const projectConfig = this._graphQLConfig.getConfigForFile(filePath);

and so on, similar to getDiagnostics

Good catch! Fixed.
Probably there is need to add some test-case that covers this piece of code

asiandrummer · 2017-07-21T10:43:30Z

packages/graphql-language-service-interface/src/__tests__/GraphQLLanguageService-test.js

 import {GraphQLLanguageService} from '../GraphQLLanguageService';

 const MOCK_CONFIG = {
-  inputDirs: ['./queries'],
+  include: ['./queries/**'],


asiandrummer · 2017-07-21T10:45:52Z

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

  ): Promise<Map<string, FragmentInfo>> => {
    // This function may be called from other classes.
    // If then, check the cache first.
-    const rootDir = graphQLConfig.getRootDir();
+    const rootDir = projectConfig.configDir;


I think you forgot to do a getter for configDir in https://github.com/graphcool/graphql-config/blob/master/src/GraphQLProjectConfig.ts, which makes me feel a little uncomfortable. If possible could we be consistent and use the proper getter? Not really a blocker though

Agree, added getter for configDir.

asiandrummer · 2017-07-21T10:46:53Z

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

-        excludeDirs.every(
-          exclude => !fileInfo.filePath.startsWith(path.join(rootDir, exclude)),
-        ));
+      .filter(fileInfo => projectConfig.includesFile(fileInfo.filePath));


This is also a bug fix I think - thanks for that

asiandrummer · 2017-07-21T10:59:34Z

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

-
-    const schemaFileExt = path.extname(schemaPath);
-    let schema;
+    let schemaSDL;


I'm worried of the performance of this approach to generate GraphQL schema. From what I understand, this approach will read the file, build GraphQLSchema object, print the schema in SDL format, and build it again. Last time I checked, buildSchema() takes around several seconds with 4~5k types available, and with extendSchema() being slow as well, this is definitely a hit with the performance.

I guess this is one of the ways to supprot .json format, which previously had bugs with my implementation to append the custom directives in SDL format to content string. One easy performance optimization we can make is to optionally perform custom directive extension when customDirective extension entry is available, with extendSchema, not buildSchema.

Great idea! I refactored code to use extendSchema when customDirective extension entry is available

Just one thing. It appeared that extendSchema loses directive description. I've already opened a PR (#961) on graphql-js to fix that but had to change "" to undefined in one test case for the time being (until my PR is merged)

asiandrummer · 2017-07-21T11:02:12Z

Really amazing work @RomanGotsiy!

RomanHotsiy · 2017-07-21T12:23:46Z

Thanks, @asiandrummer!
So far I have fixed all the issues you've spotted!

Let me know if there is anything else

asiandrummer · 2017-07-21T14:36:11Z

lgtm, cc @wincent for visibility
@RomanGotsiy what are you thoughts on the comment I made above about include and exclude?

I just realized that you've changed includeDirs and excludeDirs to include and exclude. I probably missed this while checking out the PR from graphql-config repo - is it too late to revert back to includeDirs and excludeDirs? I think include and exclude might be too generic to specify what it wants to accomplish.

Since I'm late to suggest this change, if you've already made a lot of PRs to apply this to other repos, I'm fine with writing a function to enable backward-compatibility.

RomanHotsiy · 2017-07-21T16:09:57Z

what are you thoughts on the comment I made above about include and exclude?

oops, missed that.

I think include and exclude might be too generic to specify what it wants to accomplish.

I partly agree. But at the same time globs are much more powerful. Let's say I want to exclude all the __test__ directories down to the directories tree. Using glob I can set:

"exclude": "**/__test__/**"

while using excludeDirs I will have to specify each folder + each time I create a new one I have to add it to the config.

On the other hand globs are harder to work with. + there is no simple and reliable way to convert glob to list of dirs. Does fb-watchman support globs?

asiandrummer · 2017-07-21T16:58:08Z

@RomanGotsiy sorry - I meant only the name, not the functionality :D I love the glob support!
includeDirs -> include and excludeDirs -> exclude might have been too aggressive of a change, at least for graphql-language-service. Again I do love the glob support, but could we just change the names back to includeDirs and excludeDirs?

RomanHotsiy · 2017-07-21T17:05:47Z

But if a user specifies glob "test/**/*.graphql" which is basically a file glob then includeDirs name doesn't work.
But I see now why include may be a too generic name. How about includePattern/excludePattern or includeGlob/excludeGlob ?

asiandrummer · 2017-07-21T17:12:01Z

Or maybe we could name them includes/exclude - sounds more functional.

RomanHotsiy · 2017-07-21T17:20:51Z

Yes! I love this idea 👍 I will update specification/implementation and this PR to use includes and excludes.

btw, You probably missed one s (includes/exclude <- here) in the previous comment :)

RomanHotsiy · 2017-07-24T09:42:51Z

Hey @asiandrummer,

I've updated graphql-config and this PR to use includes/excludes

schickling · 2017-07-28T12:29:21Z

Any update on this @asiandrummer? :)

asiandrummer · 2017-07-31T18:37:14Z

Sorry I've been away - thanks for the contribution!

schickling requested a review from asiandrummer July 20, 2017 14:34

RomanHotsiy mentioned this pull request Jul 21, 2017

Update graphql-js dependency and fix tests #129

Merged

asiandrummer reviewed Jul 21, 2017

View reviewed changes

RomanHotsiy added 2 commits July 21, 2017 12:59

Remove internal graphql-language-service-config

6ca95b9

Switch to graphql-config

1db5354

asiandrummer suggested changes Jul 21, 2017

View reviewed changes

review fixes

64eca9c

include/exclude -> includes/excludes

bed3ea7

Update to latest graphql-config

8753a29

schickling mentioned this pull request Jul 29, 2017

Create PRs for GraphQL plugins kamilkisiela/graphql-config#27

Closed

13 tasks

asiandrummer merged commit 99c23d4 into graphql:master Jul 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to graphql-config #130

Switch to graphql-config #130

RomanHotsiy commented Jul 20, 2017 •

edited

Loading

asiandrummer commented Jul 21, 2017

asiandrummer commented Jul 21, 2017

RomanHotsiy commented Jul 21, 2017 •

edited

Loading

asiandrummer commented Jul 21, 2017

asiandrummer Jul 21, 2017

asiandrummer left a comment •

edited

Loading

asiandrummer Jul 21, 2017

asiandrummer Jul 21, 2017

RomanHotsiy Jul 21, 2017

asiandrummer Jul 21, 2017

asiandrummer Jul 21, 2017

RomanHotsiy Jul 21, 2017

asiandrummer Jul 21, 2017

asiandrummer Jul 21, 2017

RomanHotsiy Jul 21, 2017

asiandrummer commented Jul 21, 2017

RomanHotsiy commented Jul 21, 2017

asiandrummer commented Jul 21, 2017

RomanHotsiy commented Jul 21, 2017

asiandrummer commented Jul 21, 2017

RomanHotsiy commented Jul 21, 2017 •

edited

Loading

asiandrummer commented Jul 21, 2017

RomanHotsiy commented Jul 21, 2017

RomanHotsiy commented Jul 24, 2017

schickling commented Jul 28, 2017

asiandrummer commented Jul 31, 2017

		@@ -103,10 +98,7 @@ export class GraphQLLanguageService {
		const appName = this._graphQLConfig.getAppConfigNameByFilePath(filePath);

Switch to graphql-config #130

Switch to graphql-config #130

Conversation

RomanHotsiy commented Jul 20, 2017 • edited Loading

asiandrummer commented Jul 21, 2017

asiandrummer commented Jul 21, 2017

RomanHotsiy commented Jul 21, 2017 • edited Loading

asiandrummer commented Jul 21, 2017

Choose a reason for hiding this comment

asiandrummer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asiandrummer commented Jul 21, 2017

RomanHotsiy commented Jul 21, 2017

asiandrummer commented Jul 21, 2017

RomanHotsiy commented Jul 21, 2017

asiandrummer commented Jul 21, 2017

RomanHotsiy commented Jul 21, 2017 • edited Loading

asiandrummer commented Jul 21, 2017

RomanHotsiy commented Jul 21, 2017

RomanHotsiy commented Jul 24, 2017

schickling commented Jul 28, 2017

asiandrummer commented Jul 31, 2017

RomanHotsiy commented Jul 20, 2017 •

edited

Loading

RomanHotsiy commented Jul 21, 2017 •

edited

Loading

asiandrummer left a comment •

edited

Loading

RomanHotsiy commented Jul 21, 2017 •

edited

Loading