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

Expose custom scalars configuration from CLI. #2745

Closed
wants to merge 3 commits into from

Conversation

alloy
Copy link
Contributor

@alloy alloy commented May 27, 2019

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

@@ -100,6 +100,14 @@ const argv = yargs
type: 'string',
default: null,
},
customScalars: {
Copy link
Contributor Author

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);
}
}
Copy link
Contributor Author

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/**',
Copy link
Contributor Author

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.

@alloy
Copy link
Contributor Author

alloy commented May 27, 2019

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 FormattedNumber scalar, which holds either a string or a number, but afaict the existing custom scalar support does not allow something like { "FormattedNumber": "String | Int" }. Do you have similar situations and, if so, how do you solve them? If not, would a PR for something like this be accepted?

@TrySound
Copy link
Contributor

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.

@alloy alloy force-pushed the add-custom-scalar-to-cli branch from 6c723de to 5c905f7 Compare June 5, 2019 11:02
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a 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',
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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

facebook-github-bot pushed a commit that referenced this pull request Jun 11, 2019
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
@facebook-github-bot
Copy link
Contributor

@jstejada merged this pull request in 553e331.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

relay-compiler: config of customScalars for accurate flow types
4 participants