Skip to content

Add initial code to support NPM plugin module configuration lookups; … #894

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

Merged
merged 2 commits into from
May 29, 2016

Conversation

brian428
Copy link
Contributor

…add getPluginConfig() and mergeObject() methods to seed.config.ts; use getPluginConfig() for browser-sync and gulp-concat-css as an initial test. (#850)

@brian428
Copy link
Contributor Author

brian428 commented May 18, 2016

Here's a start on the getPluginConfig() approach that we discussed over on #850. A few notes:

  • To keep the imports elsewhere in the tasks simple and consistent, I just added getPluginConfig() to the SeedConfig class.
  • I also added a mergeObject() helper method to SeedConfig to make it easy to add to or override the plugin configs (or any other objects in SeedConfig, really) from within ProjectConfig.
  • As a start, I used getPluginConfig() for browser-sync and gulp-concat-css. If you think this approach is OK, other tasks can be updated to use this as well (even if, by default, no extra config is used).

Let me know what you think. Not sure if you want to merge this yet, but I figured I'd get it over to you to check out.

* Recursively merge source onto target.
*/
mergeObject( target: any, source: any ) {
var deepExtend = require('deep-extend');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be written as const deepExtend, since it is not reassigned afterwards.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not big fan of require here. I'd prefer a definition file and import the module using the import syntax where needed. Don't you think so ?

@TheDonDope
Copy link
Contributor

Hi @brian428,
i took a peek at your code and left some minor notes :)
Have a nice day!

}
}
};

/**
* Recursively merge source onto target.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

A parameter documentation would be nice :) like:

/**
 * Recursively merge source onto target.
 * @param {any} target <short description of target>
 * @param {any} source <short description of source>
 */

@ludohenin
Copy link
Collaborator

@brian428 any progress on this ?

@brian428
Copy link
Contributor Author

Sorry...progress on what? Adding more uses of getPluginConfig()? Or making these small code style changes? I was basically waiting to hear whether the approach itself is OK.

@ludohenin
Copy link
Collaborator

ludohenin commented May 21, 2016

I was basically waiting to hear whether the approach itself is OK.

Yes you're right, we forgot the essential.
LGTM :-)
You can go ahead.

Brian Kotek added 2 commits May 24, 2016 01:01
…add getPluginConfig() and mergeObject() methods to seed.config.ts; use getPluginConfig() for browser-sync and gulp-concat-css as an initial test. (mgechev#850)
@brian428
Copy link
Contributor Author

I made the formatting and style changes as mentioned above. The only change I didn't tackle is the require() for deepExtend. Partly because the same syntax is used in numerous other spots across the tasks and configs. And partly because I'm not entirely sure what you mean about the "definition file and import" (I'm still coming to grips with the new module syntax myself).

@ludohenin ludohenin added the LGTM label May 28, 2016
@mgechev
Copy link
Owner

mgechev commented May 29, 2016

@brian428 LGTM!

@mgechev mgechev merged commit 9a86bf5 into mgechev:master May 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants