Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Allow for more complex configuration of relay-compiler / babel plugin #2518

Closed
alloy opened this issue Aug 21, 2018 · 15 comments
Closed

Comments

@alloy
Copy link
Contributor

alloy commented Aug 21, 2018

While I’m not a fan of lots of configuration –and thus think that where possible we should have generally applicable defaults, such as with persisted queries– I’m starting to feel like relay-compiler + the babel plugin could do with a more advanced way of configuration, in the form of an evaluated JS config module:

export default {
  schema: "./data/schema.json",
  language: "typescript",
  artifactDirectory: "./src/__relay_artifacts__",
  customScalars: { URL: "String" },
  persistedQueries: () => {  },
}
@alloy
Copy link
Contributor Author

alloy commented Aug 21, 2018

cc @jstejada @kassens

@jstejada
Copy link
Contributor

cc @alunyov

@sibelius
Copy link
Contributor

sibelius commented Sep 5, 2018

consuming a user babel config in js is pretty easy

you can just import like a normal .js file

@bencallaway
Copy link

@alloy Is the "language" option already supported? If not how would that work? Should it compile on the fly?

@alloy
Copy link
Contributor Author

alloy commented Sep 27, 2018

@bencallaway
Copy link

bencallaway commented Sep 27, 2018

@alloy I have been using the relay-compiler-language-typesecript plugin, and very thankful for your work there btw. I was curious about the babel-plugin-relay config itself. As of 1.7.0-rc.1 it appears "language" is not a plugin option (yet? <- Partly my previous question). However, after looking back at the code I now see there's a buildCommand option that I had failed to notice before (https://github.com/facebook/relay/blob/v1.7.0-rc.1/packages/babel-plugin-relay/BabelPluginRelay.js). It would seem I can pass in my own relay-compiler --language typescript and the compilation will occur on the fly. (I am currently doing this with a separate --watch process while developing.

In any case I'm for extending the config as you're proposing here

@bencallaway
Copy link

Ok, retracting some of that last comment. It appears that buildCommand does not handle compilation on the fly, it's just used for error reporting. I'll need to spend some more time looking through the plugin's source.

@alloy
Copy link
Contributor Author

alloy commented Sep 28, 2018

Oh I see. No currently it remains a two-step process. relay-compiler should be used to generate the artefact files, the babel plugin to switch out the graphql tags with the corresponding artefacts.

@josephsavona
Copy link
Contributor

Initially I was skeptical of this - configuration can quickly get out of hand especially if configuration values can come from too many different places (environment variables, command line args, etc). I think we can follow metro's approach (the cosmiconfig library looks good), so long as we ensure a good user experience:

  • Warn if there are multiple potential sources of configuration in a single project (e.g. a key in package.json and a standalone foo.config.js file). Maybe cosmiconfig already does this, if not we should add that.
  • Be consistent about naming of configuration keys and the corresponding command-line overrides, and allow command-line arg versions for all arguments.
  • Add a flag that shows where configuration values are coming from.

Re implementation, it would be great to have this as a standalone package. Before sending a PR, I think the first step is to write up a quick sketch of what the API would be and an overview of the implementation, list out some test cases. We can review that before proceeding with coding.

@alloy
Copy link
Contributor Author

alloy commented Jan 11, 2019

Re implementation, it would be great to have this as a standalone package.

Do you mean in the same way how relay-compiler and react-relay are separate packages but part of this monorepo?

@josephsavona
Copy link
Contributor

Yeah, in this repo as a sibling package to relay-compiler/

@alloy
Copy link
Contributor Author

alloy commented Feb 10, 2019

…allow command-line arg versions for all arguments.

I'm going to start with PRs to make all features available through the CLI first, as that needs to get done anyways.

@AugustinLF
Copy link

Regarding config coming from different places, I know that jest use a jest-config package that centralise this logic. You can find it here. It was explained by @cpojer in a video where they explained the overall architecture of jest.

@ivosabev
Copy link

ivosabev commented Apr 1, 2019

I believe this is related: #2702

@alloy
Copy link
Contributor Author

alloy commented May 27, 2019

Be consistent about naming of configuration keys and the corresponding command-line overrides

@josephsavona I noticed that we have camelCase and kebab-case options currently. camelCase would make consistency between CLI and JS configuration easier and probably more natural, but kebab-case is ofc more natural to CLIs. Should we choose one case and, if so, which one?

facebook-github-bot pushed a commit that referenced this issue Jun 11, 2019
Summary:
Closes #2162.

In preparation of #2518, as requested by josephsavona we should have CLI versions of each possible argument, hereby CLI support for custom scalars.

I ended up going with the dot-notation syntax of yargs, which seems a little foreign (to me), but it comes builtin with yargs. Let me know what you think.

E.g.

```
$ relay-compiler --customScalars.URL=String --customScalars.Date=String
```
Pull Request resolved: #2745

Reviewed By: kassens

Differential Revision: D15724870

Pulled By: jstejada

fbshipit-source-id: 62d7a8766064c9355dad07ae7a94251fa4d047b6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants