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.

Sign up for GitHub

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

Jump to bottom

configure whether plugins should be nested or not. #126

Merged
merged 6 commits into from
Oct 31, 2016
Merged

configure whether plugins should be nested or not. #126

merged 6 commits into from
Oct 31, 2016

Conversation

bretkikehara
Copy link
Contributor

@bretkikehara bretkikehara commented Oct 15, 2016 •
edited
Loading

allow user to configure whether plugin should be referenced using its package. Default behavior preserves package scope nesting.

maintainScope = true

var x = require('gulp-load-plugins')({
   maintainScope: true,
});
x.myco.testPlugin();

maintainScope = false

var x = require('gulp-load-plugins')({
   maintainScope: false,
});
x.testPlugin();

@jackfranklin
Copy link
Owner

Awesome PR, thank you, and great that it's not a breaking change!

Do you fancy adding this to the README and also yourself to the package.json contributors?

@bretkikehara
Copy link
Contributor Author

@jackfranklin What do you think about renaming scoped? I realized that using scoped may be too confusing b/c there is an existing scope option. LMK, then I will update the readme.

@jackfranklin
Copy link
Owner

Hey sorry this is such a slow reply!

I was thinking you could go for maintainScope or something similar? WDYT?

@bretkikehara
Copy link
Contributor Author

@jackfranklin Sounds good. PR has been updated to maintainScope

jackfranklin
jackfranklin reviewed Oct 26, 2016
View reviewed changes
README.md
@@ -68,7 +68,7 @@ gulpLoadPlugins({
rename: {}, // a mapping of plugins to rename
renameFn: function (name) { ... }, // a function to handle the renaming of plugins (the default works)
postRequireTransforms: {}, // see documentation below
scoped: true // loads all npm scopes like non-scoped packages
maintainScope: true // toggles loadin all npm scopes like non-scoped packages
Copy link
Owner

Choose a reason for hiding this comment

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

tiny typo here loadin

jackfranklin
jackfranklin reviewed Oct 26, 2016
View reviewed changes
README.md
@@ -118,19 +118,19 @@ Note that if you specify the `renameFn` options with your own custom rename func

## npm Scopes

`gulp-load-plugins` comes with [npm scope](https://docs.npmjs.com/misc/scope) support. By default, the scoped plugins are accessible through an object on `plugins` that represents the scope. When `scoped = false`, the plugins are availble in the top level just like any other non-scoped plugins.
`gulp-load-plugins` comes with [npm scope](https://docs.npmjs.com/misc/scope) support. By default, the scoped plugins are accessible through an object on `plugins` that represents the scope. When `maintainScope = false`, the plugins are availble in the top level just like any other non-scoped plugins.
Copy link
Owner

Choose a reason for hiding this comment

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

typo: availble

@jackfranklin
Copy link
Owner

Looks great! Just a couple of typos to fix. Are you comfortable rebasing this over master? It should be a fairly straightforward conflict to solve. If not no worries, I can do that.

@bretkikehara
Copy link
Contributor Author

fixed typos and rebased

@jackfranklin
Copy link
Owner

Wicked! One final thought - what happens if I depend on foo and @scope/foo, but turn maintainScope to false? We should probably error.

@bretkikehara
Copy link
Contributor Author

How about logging a warning? imo 99% of the time, if foo and @scope/foo are dependencies, then there is something wrong before runtime. I think it is safe to assume that packages with the same name fulfill the same functionality.

When logging a warning, I believe it would be easy to output a URL to a wiki page which defines how to resolve this issue. If an error is thrown, then I believe that there will be a lot of GitHub issues created for why isn't foo and @scope/foo plugins loading? when a gulp's error handler hasn't been defined.

@jackfranklin
Copy link
Owner

But if we log a warning and continue, Gulp will continue were foo is pointing to only one of the plugins, which will cause random errors and failures anyway? We should instead bail out and let the user know, IMO.

@bretkikehara
Copy link
Contributor Author

actually looking over the existing code, an error would already be thrown https://github.com/jackfranklin/gulp-load-plugins/blob/master/index.js#L92-L95.

@jackfranklin
Copy link
Owner

Perfect, thank you!

@jackfranklin jackfranklin merged commit e9e5e67 into jackfranklin:master Oct 31, 2016
@jackfranklin
Copy link
Owner

I'll get a new release out soon :)

@jackfranklin
Copy link
Owner

Out in 1.4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackfranklin jackfranklin jackfranklin left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bretkikehara @jackfranklin