Add relay-config package for centralised configuration #2746

alloy · 2019-05-28T18:41:00Z

Note: Includes #2745, so that should probably be merged first and then this can be rebased.

I tried multiple times to come up with a doc describing the integration a few times, but then yesterday I had a whole day and figured I’d just write a implementation to show what it could look like. Please feel free to poke holes as much as you need.

What this change does:

Add a new relay-config package
Usage of the package is optional. If installed it will be picked-up, otherwise relay will function as it does today.
RelayConfig relies on cosmiconfig to do its bidding. It’s configured to load from:
- a relay key in package.json
```
{
  "relay": {
    "src": "./src"
  }
}
```
- a relay.config.json file
```
{
  "src": "./src"
}
```
- or a relay.config.js file
```
module.exports = {
  src: "./src",
}
```
It accepts all the same configuration as the CLI does. (As a bonus, I typed the configuration object and the Yargs options object.)
Additionally, when using the relay.config.js file, a configuration entry like the language plugin also accepts an actual function:
```
const typescript = require("relay-compiler-language-typescript")

module.exports = {
  language: typescript,
}
```
In the future, other entries such as persistedQueries and customScalars could also be configured as such and allow for project specific setup.
It is used by all pieces of Relay that take configuration–i.e. relay-compiler and babel-plugin-relay. For instance, previously one would have needed to specify artifactDirectory on the CLI to relay-compiler and the same setting in .babelrc for babel-plugin-relay. This can now be reduced to a single centralised setting.

Additional external tooling can also start relying on this. For instance, the vscode-apollo extension, with this change, will allow for custom GraphQL validation rules to surface in the UI. Leveraging the centralised config allows one to surface relay-compiler specific validations without needing to take in the same configuration in a different place yet again.
Finally, this also adds some previously missing test coverage around language plugin loading.

alloy · 2019-05-28T19:06:02Z

For our apps, this:

$ relay-compiler \
  --src ./src \
  --schema data/schema.graphql \
  --language typescript \
  --artifactDirectory ./src/__generated__ \
  --persist-output ./data/complete.queryMap.json \
  --exclude '**/node_modules/**,**/__mocks__/**,**/__generated__/**'

{
  "plugins": [
    ["relay", { "artifactDirectory": "./src/__generated__" }]
  ]
}

…is replaced with:

module.exports = {
  src: "./src",
  schema: "./data/schema.graphql",
  language: "typescript",
  artifactDirectory: "./src/__generated__",
  persistOutput: "./data/complete.queryMap.json",
  exclude: ["**/node_modules/**", "**/__mocks__/**", "**/__generated__/**"],
}

{
  "plugins": ["relay"]
}

alloy · 2019-05-28T19:09:41Z

packages/relay-compiler/bin/RelayCompilerMain.js

-        languagePlugin = languagePlugin.default;
+    let languagePlugin: LanguagePlugin;
+    if (typeof language === 'string') {
+      const pluginPath = path.resolve(process.cwd(), language);


This module loading from a path could probably be deprecated with the ability to just pass a function using a relay.config.js file.

yeah, let's do that. we can deprecate this in a follow-up and delete in a later release

zth · 2019-05-28T19:35:33Z

This is so great @alloy! Centralized configuration, yummies!

alloy · 2019-05-30T06:56:25Z

gulpfile.js

+        noMinify: true, // Note: uglify can't yet handle modern JS
+      },
+    ],
+  },


(Cargo-culted this from the test utils package.)

josephsavona

Thank you so much for implementing this, it's really exciting and should make the compiler and babel plugin easier to use. See questions/comments but overall looks great.

packages/relay-compiler/bin/__tests__/RelayCompilerMain-test.js

packages/babel-plugin-relay/BabelPluginRelay.js

josephsavona · 2019-05-30T19:21:40Z

packages/relay-compiler/bin/RelayCompilerMain.js

-        languagePlugin = languagePlugin.default;
+    let languagePlugin: LanguagePlugin;
+    if (typeof language === 'string') {
+      const pluginPath = path.resolve(process.cwd(), language);


yeah, let's do that. we can deprecate this in a follow-up and delete in a later release

josephsavona · 2019-05-30T19:26:38Z

packages/relay-compiler/bin/RelayCompilerMain.js

-  const reporter = new ConsoleReporter({
-    verbose: options.verbose,
-    quiet: options.quiet,
+  const watchman = config.watchman && (await WatchmanClient.isAvailable());


hmm the previous name made it clear that this wasn't identical to config.watchman - the logic has actually changed, see comment below

I agree on the name being better–perhaps we can rename the option–but after having added test coverage for all behaviour and looking at the old behaviour (current master) I can’t spot any difference in logic.

useWatchman was determined based on options.watchman and if the client is available

relay/packages/relay-compiler/bin/RelayCompilerMain.js

Line 195 in 69a2626

const useWatchman = options.watchman && (await WatchmanClient.isAvailable());

useWatchman was from then on only used, options.watchman wasn’t ever used for anything other substantive.

Additionally, while adding full test coverage, I also noticed that previously there would be no error thrown if one were to enable watch and watchman but the client isn’t available. I changed that now, so that should improve the UX of the CLI slightly.

josephsavona · 2019-05-30T19:27:31Z

packages/relay-compiler/bin/RelayCompilerMain.js

      getFileFilter: sourceModuleParser.getFileFilter,
      getParser: sourceModuleParser.getParser,
      getSchema: () => schema,
-      watchmanExpression: useWatchman
+      watchmanExpression: config.watchman


revert? we should only pass a watchman expression if watchman is available, not just if config.watchman is set.

josephsavona · 2019-05-30T19:27:35Z

packages/relay-compiler/bin/RelayCompilerMain.js

        ? buildWatchExpression(sourceSearchOptions)
        : null,
-      filepaths: useWatchman
+      filepaths: config.watchman


josephsavona · 2019-05-30T19:27:44Z

packages/relay-compiler/bin/RelayCompilerMain.js

      getParser: DotGraphQLParser.getParser,
      getSchema: () => schema,
-      watchmanExpression: useWatchman
+      watchmanExpression: config.watchman


same here and below

josephsavona · 2019-05-30T19:43:10Z

One more thought: should we only support relay.config.js, or at least heavily encourage it in the docs? Several useful features like persisted queries only work with a function, and it seems good to encourage people to use a config format that they won't have to change later when they realize they want some feature.

alloy · 2019-05-30T20:45:19Z

I’m up for that 👍 I’ll follow up with a doc PR first.

alloy · 2019-05-31T15:20:01Z

packages/relay-compiler/bin/RelayCompilerMain.js

+  config = getPathBasedConfig(config);
+  config = await getWatchConfig(config);
+
+  const codegenRunner = module.exports.getCodegenRunner(config);


Other than adding optional function parameters to these functions to allow for DI, this appears to be a suggestion to being able to mock functions that reside in the same module as the caller jestjs/jest#936 (comment).

I have no real preference for one or the other, let me know if you do.

alloy · 2019-06-03T12:49:31Z

Btw here’s a PoC of how having this unified config interface will allow other tooling to hook in better with one’s relay setup–in this case we can have (and share with the community) a generic vscode-apollo config: https://github.com/artsy/emission/blob/b7f138d2b4311d0eed5923b39219e124a6a96a65/apollo.config.js

(I would have loved to include validation rules about eg operation/fragment names having to start with the name of the module they reside in, but I just noticed those aren’t proper validation rules, but operate directly on the parsed GraphQL tags 😞.)

alloy · 2019-06-03T13:15:23Z

One more thought on my end, if we were to only allow configuration through relay.config.js at some point, does that meant that perhaps we can move some of the configuration validation and expansion of path globs into relay-config as well?

That would allow other tooling relying on the configuration to get the same quality as relay-compiler and babel-plugin-relay would and alleviate e.g. this.

alloy · 2019-06-03T13:23:53Z

As noted here, the CI failures exist on master as well.

Just in case somebody ran `yarn install` in one of those directories.

…function

…available

facebook-github-bot

@jstejada has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@jstejada has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2019-06-11T18:38:52Z

@jstejada merged this pull request in d3ec68e.

enisdenjo · 2019-06-18T12:46:37Z

Maybe I am doing it wrong, but the config file is not picked up for me...

What I did:

Installed relay-config alongside all other Relay deps (all at v5.0.0)
Created relay.config.js at the root of my project (same level as package.json)
Having the config in package.json changes nothing...
relay.config.js has the same contents as @alloy's comment
Added a relay script in package.json which just does: relay-compiler
Running the script results in: Error: --schema path does not exist: [...]

Any ideas? Thanks! :)

enisdenjo · 2019-06-18T13:09:51Z

Never mind guys, I was searching for schema.graphql while I had schema.json... Silly me 😬

facebook-github-bot added the CLA Signed label May 28, 2019

alloy commented May 28, 2019

View reviewed changes

alloy commented May 30, 2019

View reviewed changes

josephsavona reviewed May 30, 2019

View reviewed changes

alloy commented May 31, 2019

View reviewed changes

alloy force-pushed the add-relay-config branch from 10e60a9 to 292cb1c Compare June 3, 2019 13:10

alloy added 15 commits June 5, 2019 13:02

[RelayCompilerMain] Refactor tests

b85c928

[RelayCompilerMain] Pass custom scalars to file writer

952a19b

[gulp] Ignore packages/*/node_modules

5c905f7

Just in case somebody ran `yarn install` in one of those directories.

[RelayCompilerMain] Rename options to config

c648d5b

[RelayCompilerBin] Type options as per Config

41e345a

[RelayConfig] Initial import

6ef6724

[RelayCompilerBin] Use relay-config, if installed

4f4b4b6

[BabelPluginRelay] Use relay-config, if installed

cc1c44c

[RelayCompilerMain] Add language plugin loading tests

a1d530c

[RelayCompilerMain] Allow external config to specify language plugin …

527fd77

…function

[RelayCompilerMain] Follow existing test decsription pattern

2d8a70a

[RelayCompilerMain] Cover all watch related config behaviour

074efda

[RelayCompilerMain] Throw when enabling watch without watchman being …

e996a7d

…available

[RelayCompilerMain] Follow bin defaults in tests

9ad6486

[RelayCompilerMain] Small cleanup

8dd5f9c

alloy force-pushed the add-relay-config branch from 292cb1c to 8dd5f9c Compare June 5, 2019 11:05

facebook-github-bot reviewed Jun 7, 2019

View reviewed changes

facebook-github-bot closed this in d3ec68e Jun 11, 2019

facebook-github-bot added the Merged label Jun 11, 2019

alloy deleted the add-relay-config branch June 18, 2019 14:38

alloy mentioned this pull request Jun 21, 2019

[relay] Upgrade to v5 and move to use single relay config artsy/emission#1680

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add relay-config package for centralised configuration #2746

Add relay-config package for centralised configuration #2746

alloy commented May 28, 2019

alloy commented May 28, 2019

alloy May 28, 2019

josephsavona May 30, 2019

zth commented May 28, 2019

alloy May 30, 2019

josephsavona left a comment

josephsavona May 30, 2019

josephsavona May 30, 2019

alloy May 31, 2019

josephsavona May 30, 2019

josephsavona May 30, 2019

josephsavona May 30, 2019

josephsavona commented May 30, 2019

alloy commented May 30, 2019

alloy May 31, 2019

alloy commented Jun 3, 2019

alloy commented Jun 3, 2019 •

edited

Loading

alloy commented Jun 3, 2019

facebook-github-bot left a comment

facebook-github-bot left a comment

facebook-github-bot commented Jun 11, 2019

enisdenjo commented Jun 18, 2019

enisdenjo commented Jun 18, 2019

Add relay-config package for centralised configuration #2746

Add relay-config package for centralised configuration #2746

Conversation

alloy commented May 28, 2019

alloy commented May 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zth commented May 28, 2019

Choose a reason for hiding this comment

josephsavona left a 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

josephsavona commented May 30, 2019

alloy commented May 30, 2019

Choose a reason for hiding this comment

alloy commented Jun 3, 2019

alloy commented Jun 3, 2019 • edited Loading

alloy commented Jun 3, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jun 11, 2019

enisdenjo commented Jun 18, 2019

What I did:

enisdenjo commented Jun 18, 2019

alloy commented Jun 3, 2019 •

edited

Loading