allow plugins to declare an interface for their settings

[ChristianMurphy]

An initial implementation for remarkjs/remark#383 (comment)

Unified types would highly benefit from a generic here. As written, options passed along with the use() call is a place where all type safety is lost.

This makes plugins generic, defaulting to unified's Settings type.
Plugins that do not specify a settings interface should not be unaffected.

[ChristianMurphy]

/cc @Rokt33r @JamesMessinger @mike-north

[wooorm]

Options get even harder. 🤷‍♂️ Will these types support that?

There are plugins that support non-objects as settings. And there are plugins that support multiple arguments

For example,

• unified — Supports true and false as well
• remark-rehype — Supports multiple arguments, the first could be a processor
• unified-lint-rule (and lint rules created with it, such as those in remark-lint) — Supports arrays, numbers, strings

[Rokt33r]

@wooorm

There are plugins that support non-objects as settings.

This shouldn't be problem.

interface SomePluginObjectOptions {
  ...
}

type SomePluginOptions = SomePluginObjectOptions | boolean

There are plugins that support multiple arguments.

This might be difficult though... I guess we have two options.

1. Extend unified's type definition from remark-rehype. (See module-augmentation http://www.typescriptlang.org/docs/handbook/declaration-merging.html#module-augmentation)
2. Add types and documentation all possible usage of unified in here.

---

@ChristianMurphy
While writing this comment, I've noticed that we might be able to use the module-augmentation rather than adding generic to use method.

With this, the type definition of use can be defined in each plugin.

// *.d.ts in remark-parse
declare module 'unified' {
  interface Processor {
    use(RemarkParseAttacher, RemarkParseOptions): Processor
  }
}

I'll prepare some examples tonight so we could review it together.

[Rokt33r]

@ChristianMurphy @wooorm
I prepared the example. As you see, it works quite nicely.
https://github.com/Rokt33r/module-augment-for-unified

But I think using generics with module-augmentation is NOT a good idea.
https://github.com/Rokt33r/module-augment-for-unified/blob/master/node_modules/%40types/unified/index.d.ts#L8

If use accepts generics for options, augmented types become completely useless... If generics are provided, it will be prioritized than augmented types. If generics are omitted, the plugin will expect option as Settings({[key: string]: unknown}) which is almost same to unknown. So augmented types won't work again because they are sub types of Settings.

However, module-augmentation is not a silver bullet. Because we cannot extend Pluggable from external modules, PluggableList must become unknown[]... And if we use unknown[], PluginTuple becomes useless. Because unknown[] can be super types for all array/tuple types. But I think we could give up type checking for tuple as we've given up already for PluggableList.

So to consider DX, I think using module augment without generics should be better although we cannot type-check for the usage with tuple.
(It will be less verbose because we don't need to use generics. And unified won't be needed to care how other modules are using use method.)

[ChristianMurphy]

@Rokt33r can you help me understand the Pros of using module augmentation in this case?

Conceptually I tend to think about unified as a Strategy Pattern in a Builder Pattern.
The builder pattern allowing the chaining of use, process, etc.
The strategy pattern meaning that Unified can use anything that implements the interface Plugin or Preset.

Put another way, at a types level, Plugin and Preset run inside Unified, rather than augmenting Unified.
Generics fit this pattern pretty well, all plugins will implement the Plugin interface, and can optionally define what shape/type the settings are in.

My caution with module augmentation, is that I don't see a way to check that community modules are implementing the Plugin or Preset interface.
For example, a modified version of @Rokt33r's example repo, where the plugin isn't a function, but is random attributes/properties:

import unified = require('unified')

interface MyCustomPlugin {
  random: string;
  attributes: boolean;
}

interface MyCustomPluginOptions {
  custom?: 'strict_type'
}

declare module 'unified' {
  interface Processor {
    use(plugin: MyCustomPlugin, options: MyCustomPluginOptions): Processor
  }
}

const customPlugin: MyCustomPlugin = { random: 'string', attributes: true }

// NOTE: This is valid in TypeScript, but would error when the code is run
unified().use(customPlugin, {
  custom: 'strict_type'
})

Thoughts? 💭

[Rokt33r]

can you help me understand the Pros of using module augmentation in this case?

I think it will prevent end users from making mistake when adopting plugins because they don't need to provide generics. After they installed remark-parse, Processor will be expecting the plugin should come along with RemarkParseOptions.

And if we go with generics, type checking won't work if end users don't set generics properly.

My caution with module augmentation, is that I don't see a way to check that community modules are implementing the Plugin or Preset interface.

We still can provide interfaces and types, like Preset and Attacher, which are helping to build a new plugin. I think ensuring that the module working with unified well is the community's(Plugin publishers) responsibility, not unified's one.

For example, a modified version of @Rokt33r's example repo, where the plugin isn't a function, but is random attributes/properties:

Yeah, it is too exaggerated... I just wanted to show plugin publishers can easily augment own usage of their plugins.

[ChristianMurphy]

I think it will prevent end users from making mistake when adopting plugins because they don't need to provide generics

I'm still confused here. 😕
Can you provide an example where a user would need to provide the generic?

After they installed remark-parse, Processor will be expecting the plugin should come along with RemarkParseOptions.

Isn't this what these tests demonstrate:

processor.use(typedPlugin, typedSetting)
processor.use([typedPlugin, typedSetting])

// $ExpectError
processor.use(typedPlugin, wrongTypeSettings)
// $ExpectError
processor.use([typedPlugin, wrongTypeSettings])

That the processor will validate the settings match the expected interface?

And if we go with generics, type checking won't work if end users don't set generics properly.

It would, for example:

const implicitlyTypedPlugin = (settings?: ExamplePluginSettings) => {} 

processor.use(implicitlyTypedPlugin, typedSetting)
processor.use([implicitlyTypedPlugin, typedSetting])

// $ExpectError
processor.use(implicitlyTypedPlugin, settings)
// $ExpectError
processor.use([implicitlyTypedPlugin, settings])

works, with no Generic explicitly set.
TypeScript recognizes that implicitlyTypedPlugin implements the interface Plugin and that ExamplePluginSettings should be used for the generic without needing to be told.
I've added a more complete version of the above example to the test cases.

I think ensuring that the module working with unified well is the community's(Plugin publishers) responsibility, not unified's one.

I don't see this as an either or.
Unified can provide typings that help both plugin developers and end users.
And plugin developers can write tests to ensure their plugins work.

Yeah, it is too exaggerated... I just wanted to show plugin publishers can easily augment own usage of their plugins.

It may be over exaggerated, but it demonstrates a real problem.
It encourages plugin developers to overload Unified with typings that may or may not match what Unified can actually do.

It is also more verbose

import unified = require('unified')

interface MyCustomPlugin {}

interface MyCustomPluginOptions {
  custom?: 'strict_type'
}

declare module 'unified' {
  interface Processor {
    use(plugin: MyCustomPlugin, options: MyCustomPluginOptions): Processor
  }
}

const customPlugin: MyCustomPlugin = {}

// ===================
// VS
// ===================

interface MyCustomPluginOptions {
  custom?: 'strict_type'
}

const implicitlyTypedPlugin = (settings?: MyCustomPluginOptions) => {} 

[Rokt33r]

@ChristianMurphy Hmm.. I changed my mind while I was writing this comment. Let's go with generics now.
I thought module augment would be easier for end users because they don't need to know how Attacher or Preset looks like.
But, with generics, they still don't need to know about it either. What they need to know is just providing generics to use() method...And I don't think it is a big deal as long as we provide documentation properly. So I don't mind if we are not going to adopt module augment.

Anyway, if we are going to use generics for use() method, we still need to deal with the case of multiple arguments. So if we still don't want to use module augment for this case, I guess we should overload like the below example.

use<T1, T2>(plugin: Plugin<T1, T2>, options1: T1, options2: T2)

Or should we module augment for this kind of special case?

[ChristianMurphy]

we still need to deal with the case of multiple arguments

@wooorm is it possible for the number of parameters to exceed 2?
Is there any plugin that does this today?

I guess we should overload like the below example.

use<T1, T2>(plugin: Plugin<T1, T2>, options1: T1, options2: T2)

I like this idea. 👍
Let's support 2 params for now, and in the case where plugins need more than 2 params, module augmentation may be the answer.

[Rokt33r]

@ChristianMurphy @wooorm

The changes look good to me. Now the only thing we need to decide is how many argument types should provide.

I think, as long as we don't have any plan to limit the number of options, we should provide like 4 or 5 argument types.

[wooorm]

we still need to deal with the case of multiple arguments

@wooorm is it possible for the number of parameters to exceed 2?
Is there any plugin that does this today?

— @ChristianMurphy

Yes, it is possible that there are third and other parameters. On the API side, unified and the engine support it, but it’s not used by the plugins developed inside the collective. The second parameter is used, such as by remark-rehype, and the engine passes a second parameter, which is only used by remark-validate-links. I’m not very happy with how second parameter of the engine works, and would like to change it (maybe in “uniflow”?)

[Rokt33r]

@wooorm

I’m not very happy with how second parameter of the engine works
I agree. One parameter should be enough.

We definitely could do like:

interface RemarkRehypeOptions extends ToHASTOptions {
  destination: Processor
}

So if we really want to modify the current behavior in future, we could merge this pr with two option params for now while making it deprecated.
Btw, how should we decide the modification? Should I create an issue first so we could do some poll?

[ChristianMurphy]

@wooorm @Rokt33r So I'm hearing a few things:

1. Two parameters should be enough for the use cases we know about, this PR currently supports two params, more than two parameters can be handled by module extension.
2. We should open issue(s) to deprecate and remove usage of the second parameter.
3. After 2. is completed the second parameter can be removed (maybe a major release?).

Is this accurate?
Is this PR ready to be merged?

[wooorm]

1 is accurate!

2 and 3 are not what I meant to say above. I meant to say that I don‘t like the engine passing a second argument. Because it interferes with plugins accepting other arguments.

I do think this PR is ready to go, and we could talk more about 2 and 3 somewhere else.

[wooorm]

How should this be released?

[ChristianMurphy]

Existing plugins should continue to work, they now have the option to declare a settings interface, if they don't the default generic matches the behavior from before (type unknown).
I'd lean toward marking this as SemVer minor, a feature release.