RFC: implement GraphQLConfig as the proposed protocol suggests #34

Closed
wants to merge 2 commits into
from

Projects

None yet

2 participants

@asiandrummer
Contributor

graphcool/graphql-config#20 proposes a GraphQL configuration protocol - this PR implements the said protocol and improves inline documentations

WIP - submitting a PR for an early feedback. I will include a test suite soon.

@asiandrummer asiandrummer implement GraphQLConfig as the proposed protocol suggests
748eb3d
@asiandrummer asiandrummer requested a review from wincent Feb 14, 2017
@asiandrummer asiandrummer added tests for GraphQLConfig, and modified other code/tests to match
edc1104
@wincent

Haven't finished looking at this yet (have to run off to a meeting) but this looks good.

I didn't realize we were going to change the name from .graphqlrc to .graphqlconfig. That will be a breaking change, so maybe we should support both for now with a deprecation warning for people to rename to the new name.

I'll come back and finish reviewing this later.

+ this._config[APP_EXTENSIONS_NAME][appName].excludeDirs
+ ) {
+ return this._config[APP_EXTENSIONS_NAME][appName].excludeDirs;
+ }
}
@wincent
wincent Feb 14, 2017 Contributor

Probably worth extracting this pattern into a separate function.

getConfig<T: any>(keyName: string, appName: ?string, defaultValue: T = []): T {
  if (
    appName &&
    this._config[APP_EXTENSIONS_NAME] &&
    this._config[APP_EXTENSIONS_NAME][appName] &&
    this._config[APP_EXTENSIONS_NAME][appName][keyName]
  ) {
    return this._config[APP_EXTENSIONS_NAME][appName][keyName];
  }
  return this._config[keyName] || defaultValue;
}

And then using it to implement getExcludeDirs and getIncludeDirs etc.

@wincent
wincent Feb 14, 2017 Contributor

Or something like this (you get the idea).

+ schemaPath?: string, // may be .json or .graphql
+ schemaUrl?: string,
+ // If env is specified, use one of the app configs in here
+ env?: GraphQLConfigurationEnvs,
@wincent
wincent Feb 14, 2017 Contributor

Support for this seems unimplemented now, so should we leave this out for now?

@asiandrummer
asiandrummer Feb 18, 2017 Contributor

This also describes the configuration protocol, so I think it'd be good to have it even without the implementation

+ let config;
+ beforeEach(async () => {
+ config = await getGraphQLConfig(CONFIG_DIR);
+ });
@wincent
wincent Feb 15, 2017 Contributor

beforeEach here because you are planning on adding more than one it?

@wincent
Contributor
wincent commented Feb 17, 2017

Just a heads up that once #47 lands this file will have moved from src/config/GraphQLConfig.js to packages/graphql-language-service-config/src/index.js.

It's just a straight move with no other changes at this time, so hopefully should be easy to rebase this onto it just by copying it into the new place. I'm still working on the set-up of these packages, though: the package has no tests (and no test package dependencies), so I'll need to sort that out in order for the tests added in this PR to copy over cleanly.

@asiandrummer
Contributor

After attending to the comments, I've merged this commit in several different commits due to the merge conflicts && package version bump constraints. Closing this one in favor.

@asiandrummer asiandrummer deleted the config branch Feb 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment