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

Support Node typeParam in Pluggable and PluggableList #221

Closed
4 tasks done
remcohaszing opened this issue Jun 8, 2023 · 12 comments
Closed
4 tasks done

Support Node typeParam in Pluggable and PluggableList #221

remcohaszing opened this issue Jun 8, 2023 · 12 comments
Labels
🤷 no/invalid This cannot be acted upon 👎 phase/no Post cannot or will not be acted on

Comments

@remcohaszing
Copy link
Member

Initial checklist

Problem

Currently the type of the AST is any in the Pluggable and PluggableList types. I think it would be nice if they can be specified explicitly, and default unist Node.

For example, this is a nice to have

import { compile } from '@mdx-js/mdx'

compile('', {
  remarkPlugins: [
    () => (ast) => {
      // ast is of type mdast.Root
    }
  ],
  rehypePlugins: [
    () => (ast) => {
      // ast is of type hast.Root
    },
    // This is a type error. It belongs in remarkPlugins
    remarkFrontmatter
  ]
})

Solution

In https://github.com/unifiedjs/unified/blob/main/index.d.ts#L592-L606:

  /**
   * A union of the different ways to add plugins and settings.
   *
   * @typeParam PluginParameters
   *   Plugin settings.
   */
  export type Pluggable<PluginParameters extends any[] = any[], Root = Node> =
    | PluginTuple<PluginParameters, Root, Root>
    | Plugin<PluginParameters, Root, Root>
    | Preset

  /**
   * A list of plugins and presets.
   */
  - export type PluggableList = Pluggable[]
  + export type PluggableList<Root = Node> = Pluggable<any[], Root>[]

Alternatives

Keep it as-is.

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Jun 8, 2023
@wooorm
Copy link
Member

wooorm commented Jun 8, 2023

Sure, probably good to do? Don’t see big downsides?

PluggableList is complex because it could be used for remark plugins, then remark-rehype, and then more rehype plugins: it doesn’t have to be one particular AST

@ChristianMurphy
Copy link
Member

ChristianMurphy commented Jun 8, 2023

Improvements are welcome!
This is something which would be good to smoke test downstream on some remark and rehype plugins to make sure they're still compatible.

I'd also note that it would be even more ideal, if we could make the AST type for plugins variadic.
So that it can follow the chain of plugins and make sure they are combined together correct.
And/Or to make sure that the plugin options line up with what the plugin actually accepts.

There was some exploration of this in:
#92
#96
which ran into limitations of TypeScript at the time, and weren't merged, but several TypeScript major versions later, it may be possible now.

@wooorm
Copy link
Member

wooorm commented Jun 8, 2023

So that it can follow the chain of plugins and make sure they are combined together correct.

Would that mean sort of similar to how a processor has an InputTree, CurrentTree, OutputResult? So like, PluggableList would have InputTree and OutputTree, where InputTree is the tree of the first transform and OutputTree the output of the last transform?


Aside indeed: this might run into TS limitations. If we’re going to do something, I’d prefer moving to JSDoc if that’s now possible (and/or maybe clean the JS API a bit if that makes that possible?) over doing more and more complex types

@ChristianMurphy
Copy link
Member

Would that mean sort of similar to how a processor has an InputTree, CurrentTree, OutputResult? So like, PluggableList would have InputTree and OutputTree, where InputTree is the tree of the first transform and OutputTree the output of the last transform?

Exactly

Aside indeed: this might run into TS limitations. If we’re going to do something, I’d prefer moving to JSDoc if that’s now possible (and/or maybe clean the JS API a bit if that makes that possible?) over doing more and more complex types

👍

@remcohaszing
Copy link
Member Author

I think this is still not possible indeed due to TypeScript limitations. Still, it would be useful for MDX plugin arrays, where the AST type isn’t supposed to change.

Alternatively the MDX plugin types could be changed to Array<Plugin<any, mdast.Root, mdast.Root>>, Array<Plugin<any, hast.Root, hast.Root>>, and Array<Plugin<any, estree.Program, estree.Program>> respectively.

@wooorm
Copy link
Member

wooorm commented Aug 1, 2023

That alternative seems pretty nice (though: presets?). Why not do that?

@ChristianMurphy
Copy link
Member

ChristianMurphy commented Aug 2, 2023

It may be possible for a limited number of plugins:
#92 has an example which, I think, has some relevance to what you propose.
But it doesn't scale beyond a fixed number of plugins.

Alternatively the MDX plugin types could be changed to Array<Plugin<any, mdast.Root, mdast.Root>>, Array<Plugin<any, hast.Root, hast.Root>>, and Array<Plugin<any, estree.Program, estree.Program>> respectively.

This could also work

@wooorm
Copy link
Member

wooorm commented Aug 9, 2023

MDX plugin arrays

Presets and nested plugin lists could also exist there btw.

@wooorm
Copy link
Member

wooorm commented Aug 12, 2023

After once again spending days trying to improve the types somewhat, I’d say: try it out, but I don’t have high hopes. It’s just all quite complex...

@ChristianMurphy ChristianMurphy added the ☂️ area/types This affects typings label Oct 3, 2023
@wooorm
Copy link
Member

wooorm commented Sep 24, 2024

Closing as I have strong doubts this issue is currently actionable, due to TS limitations around complex types. Improvements to types are of course welcome.

@wooorm wooorm closed this as not planned Won't fix, can't repro, duplicate, stale Sep 24, 2024

This comment has been minimized.

@wooorm wooorm added 🤷 no/invalid This cannot be acted upon and removed ☂️ area/types This affects typings 🤞 phase/open Post is being triaged manually labels Sep 24, 2024

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤷 no/invalid This cannot be acted upon 👎 phase/no Post cannot or will not be acted on
Development

No branches or pull requests

3 participants