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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: JSON configuration file to control plugins #74

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

edsrzf
Copy link
Collaborator

@edsrzf edsrzf commented Dec 14, 2020

This PR contains a proposal for a JSON configuration file format for ts-migrate, as well as a basic implementation of the proposal. If the proposal is acceptable, I can write documentation and add further validation to the implementation, if necessary.

I'm completely happy to receive feedback on any level of this proposal. Don't worry about my feelings too much. 馃榾

Context

I see these problems with ts-migrate:

It should be easy to run repeatable migrations and it should be easy to tweak plugin configurations to get the desired results.

There is currently limited ability to configure plugins, and doing it purely from the command line can be cumbersome. We can add more command-line flags to increase configurability, but this makes it more cumbersome.

It should be possible to control which plugins run and the order in which they run.

Currently the only way to do this is running multiple ts-migrate migrate --plugin $plugin commands.

There should be a way to load custom or third-party plugins.

See #41. This proposal does not aim to solve this problem, but it's a step towards solving the problem.

Solution

I propose adding a JSON configuration format for ts-migrate that allows specifying which plugins run, in what order, and the options passed to them.

The migrate command has a new flag, --config/-c, which specifies the path to the configuration file. If a configuration file is specified, other flags are ignored.

The top-level configuration object has two properties:

  • plugins - a JSON array of plugins to run. Each plugin is a JSON object, described below.
  • globalOptions (optional) - a JSON object of options which are passed to all plugins. Typically this is where anyAlias and anyFunctionAlias would be defined, but technically any option may be defined here.

A plugin object has two properties:

  • name: The name of the plugin. For example, strip-ts-ignore.
  • options (optional): The options to be passed to the plugin. Options specified here override globalOptions.

Downsides

  • Plugins may have to be more defensive about validating the options that come in, since they may come from a configuration file which is not type-checked.
  • Plugins must use JSON-compatible values for their options. (All plugins currently do.)
  • Currently the reignore command cannot be expressed in terms of a configuration object due to the "higher-order" plugins it uses. (withChangeTracking, eslintFixChangedPlugin)

Schema

Here is a basic JSON schema which describes the structure of the configuration object. See also the defined TypeScript types in the implementation.

{
  "title": "Config",
  "type": "object",
  "required": ["plugins"],
  "properties": {
    "globalOptions": {
      "type": "object"
    },
    "plugins": {
      "title": "Plugin",
      "type": "array",
      "items": {
        "type": "object",
        "required": ["name"],
        "properties": {
          "name": {
            "type": "string"
          },
          "options": {
            "type": "object"
          }
        }
      }
    }
  }
}

Example

The configuration for the default migrate command would look like this:

{
  "globalOptions": {
    "//": "When --aliases tsfixme is passed",
    "anyAlias": "$TSFixMe",
    "anyFunctionAlias": "$TSFixMeFunction"
  },
  "plugins": [
    {
      "name": "strip-ts-ignore"
    },
    {
      "name": "hoist-class-statics"
    },
    {
      "name": "react-props",
      "options": {
        "shouldUpdateAirbnbImports": true
      }
    },
    {
      "name": "react-class-state"
    },
    {
      "name": "react-class-lifecycle-methods",
      "options": {
        "force": true
      }
    },
    {
      "name": "react-default-props",
      "options": {
        "//": "When --use-default-props-helper is passed",
        "useDefaultPropsHelper": true
      }
    },
    {
      "name": "react-shape-plugin"
    },
    {
      "name": "declare-missing-class-properties"
    },
    {
      "name": "explicit-any"
    },
    {
      "name": "eslint-fix"
    },
    {
      "name": "ts-ignore"
    },
    {
      "name": "eslint-fix"
    }
  ]
}

@Rudeg
Copy link
Contributor

Rudeg commented Dec 16, 2020

@edsrzf I think it's a cool idea!

Plugins may have to be more defensive about validating the options that come in, since they may come from a configuration file which is not type-checked.

Agree, + probably we should try to surface errors, instead of running with default/invalid configuration.

Wdyt about using yaml for the configuration, instead of JSON?

Example:

---
globalOptions:
  anyAlias: "$TSFixMe"
  anyFunctionAlias: "$TSFixMeFunction"
plugins:
- name: strip-ts-ignore
- name: hoist-class-statics
- name: react-props
  options:
    shouldUpdateAirbnbImports: true
- name: react-class-state
- name: react-class-lifecycle-methods
  options:
    force: true
- name: react-default-props
  options:
    useDefaultPropsHelper: true
- name: react-shape-plugin
- name: declare-missing-class-properties
- name: explicit-any
- name: eslint-fix
- name: ts-ignore
- name: eslint-fix
  • it looks cleaner :)
  • but we need to parse it, instead of just using JSON.parse

@lencioni
Copy link
Member

Generally speaking, I'm in favor of JSON (or JSONC) over YAML, especially for tools in the JS/TS ecosystem.

@edsrzf
Copy link
Collaborator Author

edsrzf commented Dec 16, 2020

I am personally not a fan of YAML, but I won't argue against it if the consensus is that we should use it.

@Rudeg
Copy link
Contributor

Rudeg commented Dec 17, 2020

Let's go with JSON then, if that's good with you. It would also be simpler in terms of parsing and consistent.
Thanks for writing a proposal and working on this, @edsrzf!

@edsrzf
Copy link
Collaborator Author

edsrzf commented Dec 20, 2020

Alright, so we're happy with JSON. We need better validation of plugin options. Is there anything else blocking this proposal?

For plugin option validation, what would you think about adding something like this to the Plugin interface?

interface Plugin<TPluginOptions = unknown> {
  // ...

  /**
   * Returns true if options is a valid options object for this plugin.
   * If options is invalid, throws an exception with details of why options is invalid.
   *
   * This method must be implemented if TPluginOptions is anything other than unknown.
   */
  validate?(options: unknown): options is TPluginOptions;
}

I don't love the "return true or throw exception" behavior but it doesn't seem to be possible to combine TypeScript type predicates with union types (eg something like (options is TPluginOptions) | string). I'm open to other suggestions. I can make a separate PR for this.

@Rudeg
Copy link
Contributor

Rudeg commented Dec 21, 2020

@edsrzf

Alright, so we're happy with JSON. We need better validation of plugin options. Is there anything else blocking this proposal?

I think we're all good here!

For plugin option validation, what would you think about adding something like this to the Plugin interface?

In terms of validation, I agree with you. Your proposal is more explicit than return true or throw exception IMO. Maybe let's go with this?

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

Successfully merging this pull request may close these issues.

None yet

3 participants