-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Expose custom scalars configuration from CLI. #2745
Conversation
@@ -100,6 +100,14 @@ const argv = yargs | |||
type: 'string', | |||
default: null, | |||
}, | |||
customScalars: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re case, see #2518 (comment)
if (options.validate && result !== 'NO_CHANGES') { | ||
process.exit(101); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of these changes are to refactor configuration of the codegen runner out into a standalone function that can be tested.
@@ -260,6 +260,7 @@ const modules = gulp.parallel( | |||
'!**/__tests__/**', | |||
'!**/__flowtests__/**', | |||
'!**/__mocks__/**', | |||
'!**/node_modules/**', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I lost a bunch of time faffing around trying to find the best way to do continuous development on the package while integrated into one of our apps, which led me to at some point running yarn install
in the packages/relay-compiler
dir. After that building failed in confusing ways, so this addition is really just to guard other devs from the same silly mistake.
Note that I had to do some synthetic tests, as none of our schema’s custom scalars really maps to a single builtin GraphQL scalar type, but doing so did update the [typescript] artifacts in our app as expected. For instance, we have a |
Awesome! I want it so much. Currently to make date/time scalars type safe I have to copy/paste RelayCompilerBin and RelayCompilerMain into my project. |
Just in case somebody ran `yarn install` in one of those directories.
6c723de
to
5c905f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jstejada has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
'Mappings from custom scalars in your schema to built-in GraphQL ' + | ||
'types, for type emission purposes. (Uses yargs dot-notation, e.g. ' + | ||
'--customScalars.URL=String)', | ||
type: 'object', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alloy I'm getting flow errors for this type. Is it actually supported? I can't find it in the docs: http://yargs.js.org/docs/#api-optionkey-opt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
going to merge this in anyway and we can fix in a follow up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn’t exist yet, in the sense that it checks the input type, but setting it here does make it show up in the help banner and so I think it’s helpful to include already.
I did not notice any flow errors at the time, but also the relay-config PR applies custom typings to these options, so perhaps that overrides any current failures here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it! thanks! I'll just leave it with a $FlowFixMe for now then
Summary: Closes #2518 _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](https://github.com/davidtheclark/cosmiconfig) to do its bidding. It’s configured to load from: - a `relay` key in `package.json` ```json { "relay": { "src": "./src" } } ``` - a `relay.config.json` file ```json { "src": "./src" } ``` - or a `relay.config.js` file ```js 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: ```js 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](apollographql/apollo-tooling#1288), 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. Pull Request resolved: #2746 Reviewed By: josephsavona Differential Revision: D15721312 Pulled By: jstejada fbshipit-source-id: d9b76ac1866a9b0f2c2a76f65a16f79a28d4bc24
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.