Closed Rokt33r closed 5 years ago
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:
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? :thought_balloon:
A bit more research turned up https://github.com/microsoft/TypeScript/pull/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
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)
.
I've opened #62 as one possible solution that would remove extraSettings
and allow any number of settings to be passed through.
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)
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).
@wooorm mentioned in https://github.com/unifiedjs/unified/pull/58#issuecomment-509576225
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....
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.
So, I'm down with this idea. But I also admit that this is a very trivial issue so we can just ignore.