Stop allowing extraSettings

[Rokt33r]

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”?)

@wooorm mentioned in #58 (comment)

I think allowing extraSettings is giving too much unnecessary freedom.

For example, remark-rehype is accepting two optional settings, a function for a destination processor and an object for ToHast settings. So remark-rehype should check the first argument's type if it is a function or an object. And we need to provide extra types like the below....

function remark2rehype(settings?: ToHastSettings)
function remark2rehype(destination?: Processor, settings?: ToHastSettings)
function remark2rehype(destinationOrSettings?: Processor | ToHastSettings, settings?: ToHastSettings)

In my opinion, it might cause a little tiny bit of confusing for plugin providers and adopters. Haha

If we don't allow extra settings, remark-rehype could be coerced to become a bit simpler like the below example.

interface RemarkRehypeSettings extends ToHastSettings {
  destination: Processor
}

function remark2rehype(settings?: RemarkRehypeSettings)

So, I'm down with this idea. But I also admit that this is a very trivial issue so we can just ignore.

[ChristianMurphy]

I'm a bit conflicted here.

On one hand, I agree, having S, S2 and potentially S3 and beyond in the future is complex and difficult to understand.

I also agree that

function remark2rehype(settings?: ToHastSettings)
function remark2rehype(destination?: Processor, settings?: ToHastSettings)
function remark2rehype(destinationOrSettings?: Processor | ToHastSettings, settings?: ToHastSettings)

is a bit complex and might trip me up.

---

On the other hand,
I also see the value in trying to represent the JavaScript API as closely as possible,
so TypeScript adopters can leverage existing plugins without needing to extend the unified module, and dig into how the internals work.

---

One thing I've been researching is arbitrary length generic tuples (best resource so far https://stackoverflow.com/questions/49847407/typescript-generics-with-ordered-array-tuples)
Ideally we should be able to have an any number of settings and order-sensitive, strong type checking.
From what I've found so far, this may be rather complex, but but could potentially produce good results.
And might even be usable for carrying strong settings checking into the PluggableList type.

More research would be needed into the feasibility.

---

Another item I've been researching, which would be simpler, would be using a rest on the Attacher type and PluginTuple type.

/**
 * An attacher is the thing passed to `use`.
 * It configures the processor and in turn can receive options.
 *
 * Attachers can configure processors, such as by interacting with parsers and compilers, linking them to other processors, or by specifying how the syntax tree is handled.
 *
 * @this Processor context object is set to the invoked on processor.
 * @param settings Configuration
 * @param extraSettings Secondary configuration
 * @typeParam S Plugin settings
 * @typeParam P Processor settings
 * @returns Optional Transformer.
 */
interface Attacher<S = Settings, P = Settings> {
  (this: Processor<P>, ...settings: S[]): Transformer | void
}

/**
 * A pairing of a plugin with its settings
 *
 * @typeParam S Plugin settings
 * @typeParam P Processor settings
 */
type PluginTuple<S = Settings, P = Settings> = [Plugin<S, P>, ...S[]]

Which is pretty simple, and would allow for an arbitrary number of settings.
The issues I've come across with this solution:

1. it is not sensitive to settings order.
2. it can't infer multiple settings types (it needs to explicitly be passed a type union)
3. it is getting tripped up when there is both a settings object and a processor as valid (see example code below

let processor: Processor = unified()

interface ExamplePluginSettings {
  example: string
}

const typedSetting = {example: 'example'}

const pluginWithTwoSettings: Plugin<Processor | ExamplePluginSettings> = (
  processor?,
  settings?
) => {}

// these should be valid (?), but are throwing type exceptions
// not sure why yet
processor.use(pluginWithTwoSettings, processor, typedSetting)
processor.use([pluginWithTwoSettings, processor, typedSetting])
processor.use([pluginWithTwoSettings, processor])

---

Removing extraSettings and only allowing one setting could be an option.
extraSetting is only used in a couple places.
Having the typings out of sync with the code capabilities still gives me some pause.
extraSetting may trip people up, but not having it could also trip people up.

---

Thoughts? 💭

[ChristianMurphy]

A bit more research turned up microsoft/TypeScript#24897 and https://github.com/Microsoft/TypeScript/wiki/What's-new-in-TypeScript#tuples-in-rest-parameters-and-spread-expressions

If I'm reading it correctly, it means something like

type genericFunction<T extends any[]> = (...settings: T) => void

type genericTuple<T extends any[]> = [genericFunction<T>, ...T]

should be possible, and typescript will automatically infer the types.

Currently running this is blocked by a type exception: A rest element type must be an array type which should (?) be possible thanks to extends any[]
https://www.typescriptlang.org/play/index.html#code/C4TwDgpgBA5hB2EBOBLAxgMQK7zcFA9vADwAqUEAHsAgCYDOUAhvCANoC6AfFALxQAKAHQj6EYPngx6ALiikAlHx4A3AiloBYAFA7QkWAmTpSWMABsIZCtTqMW7bnyhs4iVJhx5CJUlwA0UCJCpBw6QA

[wooorm]

One thing to add: if this is a problem for types, well, that’s something to fix in plugins. They should use simpler APIs with less arguments.

That does not mean unified itself should (or should not) allow multiple arguments.
I like that unified allows for so many .use cases, it makes sense to me. Making it easy to use plugins is essentially all unified does. It makes sense that .use(plugin, a, b, c) turns into plugin(a, b, c).

[ChristianMurphy]

I've opened #62 as one possible solution that would remove extraSettings and allow any number of settings to be passed through.

[Rokt33r]

I like that unified allows for so many .use cases, it makes sense to me.

I see. I'm glad to know what do you want.

I guess I'm just a different person who prefers less usage options. Haha
If I made unified, processor wouldn't accept any settings for plugins because they could be provided like, unified().use(plugin(a, b, c)).(Although we would lose global settings...)
But I'm not an author of unified and I'm just here for improving unified, not for making my own package.😄

Anyway, I think this issue can be closed soon.(now or after merging #62)

[wooorm]

I guess I'm just a different person who prefers less usage options

I think that makes sense! And I would agree, but there are also good reasons why things exist the way they are. Then again, it’s important that the API surface is reviewed often to see if things can change to be simpler.

Most of the different ways of use are inspired by other similar projects (such as ware, postcss, rework, metalsmith), but also added over the years due to new features (the engine, presets).
the main reasons that we do A .use(plugin, a, b, c) instead of B .use(plugin(a, b, c)) is because we can reconfigure plugin (needed mostly in presets), and because we can pass this (the processor) to plugin as well (needed to configure a parser, a compiler).
The reason for global settings is preconfigured processors (like remark, rehype, retext). Otherwise we could drop them (although they sometimes come in handy in other cases).

Some parts are legacy, and some parts are because unified can be used to do so many things (some of which never came into existence).