Skip to content

Make Base.plugin() accept an array of plugins instead of requiring the plugins to be spread #61

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

Closed
gr2m opened this issue Jul 17, 2021 · 3 comments

Comments

@gr2m
Copy link
Owner

gr2m commented Jul 17, 2021

The current implementation of plugin is a result of quite a long research when we first created it. The only way we got it to work is by creating an interface Base.plugin() which requires at least one parameter, but also accepts an unlimited amount of parameters, each one being a plugin function.

The preferable API signature is

Base.withPlugins([plugin1, plugin2, ...])

Lots of things changed since the initial implementation.

@JoshuaKGoldberg maybe the improvements you too advantage of here will make this possible? Would that be something that you know is possible now or that you'd like to explore?

@JoshuaKGoldberg
Copy link
Collaborator

Sure! I think that's reasonable. At first glance, it looks like these rough changes would get most or all of the way there:

index.js:

- static plugin(...newPlugins) {
+ static plugin(newPlugins) {
    const currentPlugins = this.plugins;
    return class extends this {
      static plugins = currentPlugins.concat(
        newPlugins.filter((plugin) => !currentPlugins.includes(plugin))
      );
    };
  }

index.d.ts:

  static plugin<
    Class extends ClassWithPlugins,
    Plugins extends [Plugin, ...Plugin[]],
  >(
    this: Class,
-   ...plugins: Plugins,
+   plugins: Plugins,
  ): Class & {
    plugins: [...Class['plugins'], ...Plugins];
  } & Constructor<UnionToIntersection<ReturnTypeOf<Plugins>>>;

If backwards compatibility is of concern, it could instead take in a Plugins extends [(Plugin | Plugin[]), ...(Plugin | Plugin[])[] in the definition and use Array.flat in the implementation. Either works!

@gr2m
Copy link
Owner Author

gr2m commented Jul 17, 2021

At first glance, it looks like these rough changes would get most or all of the way there

Awesome, thank you!

I'll give this a try myself and reach out if I get stuck with specific questions and failing tests

If backwards compatibility is of concern

it's not a concern here because this is just a demo/reference project. But it might become a concern for Octokit, so that's great to know

@gr2m
Copy link
Owner Author

gr2m commented Jul 20, 2021

that's done now 👍🏼

@gr2m gr2m closed this as completed Jul 20, 2021
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

No branches or pull requests

3 participants
@gr2m @JoshuaKGoldberg and others