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

feat: allow multiple local plugins #1980

Conversation

juliuscc
Copy link
Contributor

@juliuscc juliuscc commented Jun 26, 2020

This PR allows for multiple local plugins to be loaded.

Description

The plugins are usually loaded by their name and added to an object, with their name as key. Local plugins do not have a name so they were previously saved as 'local' in the same object. If you loaded multiple local plugins the value was overwritten.

This PR saves all local plugins on a unique key. They are called local-0, local-1, etc.

Motivation and Context

First of all you can have multiple local plugins. This means that you can export and import them without having to merge them. This also means that you can include multiple shareable plugins that include local plugins, without them overwriting each other.

Usage examples

Self-explanatory.

How Has This Been Tested?

I have added a test for multiple plugins, and they are correctly loaded.

I have not tested how it works when you include multiple shared configurations with plugins or a mix of local plugins in your config and from a shared config. This is because currently, the master branch does not support plugins from shared configs in any way. When that is fixed we can add tests for this.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@juliuscc
Copy link
Contributor Author

I suggest merging #1976 before this one :)

@escapedcat escapedcat requested a review from byCedric June 27, 2020 05:43
@@ -24,6 +24,12 @@ import {pickConfig} from './utils/pick-config';
const w = <T>(_: unknown, b: ArrayLike<T> | null | undefined | false) =>
Array.isArray(b) ? b : undefined;

const generateLocalId = () => {
let id = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this redefined every time you run the method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but it is only run once. The function that is run multiple times is this one:

return () => `local-${id++}`;

It is a closure :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was considering to just declare id on row 95 and do the logic for generating an id in row 101. Do you think the code is clearer that way?

@escapedcat
Copy link
Member

@juliuscc sorry, this got lost on the way. Might be better to just merge this in even though some questions are open. Would you mind rebasing it? Then we could merge this.

@escapedcat escapedcat closed this Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants