Restrict config properties (Implementation for Issue #92) by sttk · Pull Request #94 · gulpjs/gulp-cli

sttk · 2016-09-04T15:33:09Z

This PR is for the issue #92.

I only implemented about the properties: description, flags.help, and flags.gulpfile first.

The points that I modified from #90 are as follows:

Config properties by cli-flags take precedence over them by config files.

I made a mistake in the previous implementation to merge properties of config files over opts properties, so config files took precedence over command line flags. I modified it.

Since it would be confusing to control to change or not opts properties at the time of merging, the current code evacuates opts properties specified by cli-flags before merging, merge with config by config files, and merge with the evacuated properties at the end.
Add an object disp for display settings.

This object is used for outputing description for gulpfile and will be used for various display configurations.

For this object, the api of the versioned cli modules in lib/versioned/*/index.js are added 3rd argument.
Add validations for config properties of each config file.

And if a validation fails, outputs the error and quits immediately.
Add convert for config properties of each config file.

Because it is needed to convert a property value into an absolute path with a base directory path if the property value is a path string.
Add env to the merged targets because env.cwd, env.configPath and env.require are used for configuration of cwd, gulpfile and require.

sttk · 2016-09-04T15:38:34Z

In addition, we should consider about following points:

Specifying --cwd or --gulpfile affect each other. Is it OK to set flags.cwd or flags.gulpfile independently about specifying by config file?
--require could be specified multiple values. Should the value of flags.require be able to set a string or an array? And Should the value by config files replace or append?
Is it OK that a base path of flags.cwd and flags.gulpfile are a directory of a .gulp.* file?

sttk · 2016-09-05T14:08:45Z

index.js

+
+function customizeByEachConfig(config, opts, env, disp, filePath) {
+  try {
+    var config = require(filePath);


I found that this line is redundant with line 182. I'll remove this line or line 182.

sttk · 2016-09-06T15:44:36Z

I've removed the redundant code.

phated · 2016-09-06T18:56:28Z

@sttk any idea why the tests failed on node 6?

sttk · 2016-09-06T20:24:42Z

The cause of this error is seemed to be an accidental timeout of the test for --completion and it is thought that this is not related to this PR.

Actually, the same test for my repository is successful.

https://travis-ci.org/sttk/gulp-cli/builds/157911810

phated · 2016-09-15T23:22:31Z

@sttk Sorry for not having a chance to fully review this yet. I don't think description should be placed under a disp key and should remain at the top level. You can drop disp completely for now.

sttk · 2016-09-17T02:00:56Z

@phated disp is not a key of config or opts but an inner object to pass to versioned runners. Since the description key places under opts in #90 implementation, this key can be also specified by unspecific --description cli-flag. disp is for preventing it.

sttk · 2016-12-04T08:23:05Z

I re-created the code for this PR.

This code supports and restricts only the config properties: description, flags.silent and flags.gulpfile first.

One problem about the property flag.silent occurs. Before loading config files, which is executed at the top of a callback function of Liftoff.launch, configuration of logging is already done in toConsole function.

Tentatively, I moved toConsole function after the loading, and for the requiring logs, which are output before the loading, I used fancy-log directly .

phated · 2016-12-14T21:59:05Z

@sttk ugh, sorry that I still haven't had a chance to review this PR. Life has been hectic for me. I will try to review soon.

phated · 2016-12-27T00:22:59Z

@sttk this will need to be updated after I merge the mocha/expect changes.

phated · 2017-01-06T19:51:17Z

Feel free to rebase/update this based on the merge of #99

sttk · 2017-01-07T04:53:40Z

@phated I'll update this PR.

sttk · 2017-01-07T12:51:49Z

@phated I finished updating this PR.

phated

@sttk The tests are looking solid but I had some questions and code style changes inline.

phated · 2017-01-11T20:53:23Z

index.js

 var getBlacklist = require('./lib/shared/getBlacklist');
 var toConsole = require('./lib/shared/log/toConsole');

+var loadConfigFiles = require('./lib/shared/config/loadfiles');


rename to load-files please.

phated · 2017-01-11T20:55:44Z

index.js

-  }, handleArguments);
+  };
+
+  cli.launch(envOpts, function(env) {


This should be a named function

phated · 2017-01-11T20:56:07Z

index.js

+  cli.launch(envOpts, function(env) {
+    var config;
+    try {
+      config = loadConfigFiles(env.configFiles['.gulp'], ['home', 'cwd']);


Why does this have the chance to throw? Can it be rewritten to not throw?

This exception is caused by validation of config files. Does your comment mean that outputting error and exiting should be done in loadConfigFiles, or it is unnecessary to validate config files?

Do we need to validate the config? Could we just ignore and log a warning or error if it's the wrong type?

I don't like the throw/try/catch pattern and we shouldn't be exiting.

I got it. I'll make display errors but not exit.

phated · 2017-01-11T20:57:04Z

index.js

+    }
+
+    mergeToCliFlags(opts, config, cliOptions);
+    mergeToEnvFlags(env, config, envOpts);


Why do we pass envOpts in here?

This should be non-mutating (return a new object).

Why do we pass envOpts in here?

To exclude env property names related to command-line arguments.

This should be non-mutating (return a new object).

I got it.

To exclude env property names related to command-line arguments.

Can you explain this further?

phated · 2017-01-11T20:57:26Z

index.js

+      exit(1);
+    }
+
+    mergeToCliFlags(opts, config, cliOptions);


This should be non-mutating (return a new object).

phated · 2017-01-11T21:07:45Z

lib/shared/config/env-flags.js

+
+  if (!envOpts.configPath && cfgFlags.gulpfile) {
+    envFlags.configPath = cfgFlags.gulpfile;
+    envFlags.configBase = path.dirname(cfgFlags.gulpfile);


This should be creating a new object instead of mutating

phated · 2017-01-11T21:17:18Z

lib/shared/config/loadfiles.js

+function loadConfigFiles(configFilesBase, keysInOrder) {
+  var config = {};
+
+  var configFilePaths = keysInOrder.map(function(key) {


Does fined not do this for us automatically? It's okay if it doesn't but my mind is fuzzy on it.

Properties order in objects is not guaranteed, though Object.keys looks like returning in definition order actually. I think it is needed to specify the order explicitly.

Sorry, I meant that fined crawls the file system and only gives us the config file that matches. Or are we using it differently?

If config files are found in user home directory and project directory, both file paths are given by fined. In that case each config in ~~both~~ project dir should be given priority over each config in user home dir, I think.

Though this order might be determined by descriptions at Liftoff constructor, I want to specify it explicitly.

--
EDIT: erase the word "both"

phated · 2017-01-11T21:18:13Z

lib/shared/config/loadfiles.js

@@ -0,0 +1,33 @@
+'use strict';
+
+var get = require('lodash.get');


I'm going to be removing lodash. It's okay for us to leave it in for this implementation unless you want to find replacements

If it is good to use copy-props, I'll replace this to it.

Are we using it in other places? I'm fine with using it since you will be able to fix anything needed.

phated · 2017-01-11T21:18:47Z

lib/shared/config/loadfiles.js

+        return;
+      }
+
+      set(config, keyPath, spec.validate(value, filePath));


What is validate doing in this scenario?

Validating each config value comming from config files, and convert file path (e.g. gulpfile) from relative path from .gulp.* to absolute path.

I don't like the validate stuff. I think there's a better way to do this.

phated · 2017-01-11T21:19:15Z

lib/versioned/^3.7.0/index.js

      var tree = taskTree(gulpInst.tasks);
-      if (opts.description && isString(opts.description)) {
-        tree.label = opts.description;
+      if (config.description) {


Will this always exist now?

About config object? It will be always exists.

Is config.description always guaranteed to be undefined or a String?

Yes. It will be checked and output errors if invalid.

Put the isString check back in when the validation is removed.

sttk

I'll modify what you pointed out.

sttk · 2017-01-14T09:26:03Z

index.js

 var getBlacklist = require('./lib/shared/getBlacklist');
 var toConsole = require('./lib/shared/log/toConsole');

+var loadConfigFiles = require('./lib/shared/config/loadfiles');


sttk · 2017-01-14T09:27:45Z

index.js

-  }, handleArguments);
+  };
+
+  cli.launch(envOpts, function(env) {


sttk · 2017-01-14T09:27:53Z

index.js

+  cli.launch(envOpts, function(env) {
+    var config;
+    try {
+      config = loadConfigFiles(env.configFiles['.gulp'], ['home', 'cwd']);


This exception is caused by validation of config files. Does your comment mean that outputting error and exiting should be done in loadConfigFiles, or it is unnecessary to validate config files?

sttk · 2017-01-14T09:28:12Z

index.js

+      exit(1);
+    }
+
+    mergeToCliFlags(opts, config, cliOptions);


sttk · 2017-01-14T09:28:21Z

index.js

+    }
+
+    mergeToCliFlags(opts, config, cliOptions);
+    mergeToEnvFlags(env, config, envOpts);


Why do we pass envOpts in here?

To exclude env property names related to command-line arguments.

This should be non-mutating (return a new object).

I got it.

sttk · 2017-01-14T09:31:40Z

lib/shared/config/env-flags.js

+
+  if (!envOpts.configPath && cfgFlags.gulpfile) {
+    envFlags.configPath = cfgFlags.gulpfile;
+    envFlags.configBase = path.dirname(cfgFlags.gulpfile);


sttk · 2017-01-14T09:32:06Z

lib/shared/config/loadfiles.js

@@ -0,0 +1,33 @@
+'use strict';
+
+var get = require('lodash.get');


If it is good to use copy-props, I'll replace this to it.

sttk · 2017-01-14T09:32:18Z

lib/shared/config/loadfiles.js

+function loadConfigFiles(configFilesBase, keysInOrder) {
+  var config = {};
+
+  var configFilePaths = keysInOrder.map(function(key) {


Properties order in objects is not guaranteed, though Object.keys looks like returning in definition order actually. I think it is needed to specify the order explicitly.

sttk · 2017-01-14T09:32:50Z

lib/shared/config/loadfiles.js

+        return;
+      }
+
+      set(config, keyPath, spec.validate(value, filePath));


Validating each config value comming from config files, and convert file path (e.g. gulpfile) from relative path from .gulp.* to absolute path.

sttk · 2017-01-14T09:33:11Z

lib/versioned/^3.7.0/index.js

      var tree = taskTree(gulpInst.tasks);
-      if (opts.description && isString(opts.description)) {
-        tree.label = opts.description;
+      if (config.description) {


About config object? It will be always exists.

phated · 2017-01-25T20:47:47Z

@sttk Don't mean to pester you but do you know when you'll have a chance to work on the changes?

sttk · 2017-01-25T21:08:38Z

@phated I'll work on the changes this weekend. Why?

phated · 2017-01-25T21:26:00Z

No reason, I've just been really active on the project lately. I know how hard it is to get me to review, haha!

sttk · 2017-01-28T18:28:19Z

Since I've modified matters pointed out by you, please review.
And list flags that we will be able to specify with config files, please.

phated · 2017-04-19T18:14:14Z

@sttk I'm terrible. Sorry! Looking at this now.

phated

Sorry this took me so long to review. I promise to review quicker on your next updates.

phated · 2017-04-19T19:03:28Z

index.js

 var fs = require('fs');
 var path = require('path');
 var log = require('gulplog');
+var fancyLog = require('fancy-log');


I really don't like using fancy-log directly, it means that someone using --silent will still see the messages. Looking into re-binding the logger after we parse the config.

toConsole can be updated to remove previous listeners if called a 2nd time. You can then use it at the beginning of this file and call it again after you normalize the config.

phated · 2017-04-19T19:17:30Z

index.js

-
 cli.on('require', function(name) {
-  log.info('Requiring external module', chalk.magenta(name));
+  fancyLog('Requiring external module', chalk.magenta(name));


See above about fancy-log

phated · 2017-04-19T19:17:34Z

index.js


 cli.on('requireFail', function(name) {
-  log.error(chalk.red('Failed to load external module'), chalk.magenta(name));
+  fancyLog.error(


See above about fancy-log

phated · 2017-04-19T19:18:34Z

index.js


-// The actual logic
-function handleArguments(env) {
+function configureAndInvoke(env) {


Is this just split out due to the linting complaining about number of statements? I'd be happier increasing the limit and putting it inside handleArguments.

phated · 2017-04-19T19:20:23Z

lib/shared/config/convert-config.js

+'use strict';
+
+var path = require('path');
+var logger = require('fancy-log');


See above about fancy-log

phated · 2017-04-19T19:32:46Z

lib/versioned/^4.0.0-alpha.2/index.js

      tree = gulpInst.tree({ deep: true });
-      if (opts.description && isString(opts.description)) {
-        tree.label = opts.description;
+      if (config.description) {


Put the isString check back in when the validation is removed.

phated · 2017-04-19T19:32:51Z

lib/versioned/^4.0.0/index.js

      tree = gulpInst.tree({ deep: true });
-      if (opts.description && isString(opts.description)) {
-        tree.label = opts.description;
+      if (config.description) {


Put the isString check back in when the validation is removed.

phated · 2017-04-19T19:32:56Z

lib/versioned/^4.0.0/index.js

      tree = gulpInst.tree({ deep: true });
-      if (opts.description && isString(opts.description)) {
-        tree.label = opts.description;
+      if (config.description) {


Put the isString check back in when the validation is removed.

phated · 2017-04-19T19:33:03Z

lib/versioned/^4.0.0-alpha.2/index.js

      tree = gulpInst.tree({ deep: true });
-      if (opts.description && isString(opts.description)) {
-        tree.label = opts.description;
+      if (config.description) {


Put the isString check back in when the validation is removed.

phated · 2017-04-19T19:34:08Z

package.json

  },
  "dependencies": {
    "archy": "^1.0.0",
+    "camelcase": "^3.0.0",


This can be removed when excludeCliArgs is removed.

sttk · 2017-04-20T13:06:52Z

@phated Thanks for your reviewing. I'll address what you pointed out this week end.

… in index.js

sttk · 2017-04-22T07:45:31Z

@phated I've modified.

--
EDIT:
Sorry. I forgot to remove camelcase from dependencies. I've done.

phated · 2017-04-22T23:07:16Z

lib/shared/config/load-files.js

+
+    copyProps(require(filePath), config, function(value, name) {
+      if (name === 'flags.gulpfile') {
+        return path.resolve(path.dirname(filePath), value);


Oh interesting, this is making the path in the config file relative its location.

phated · 2017-04-22T23:07:41Z

lib/shared/config/load-files.js

+function loadConfigFiles(configFiles, configFileOrder) {
+  var config = {};
+
+  configFileOrder.forEach(function(key) {


Named function (I can make this change during merge).

phated · 2017-04-22T23:07:46Z

lib/shared/config/load-files.js

+      return;
+    }
+
+    copyProps(require(filePath), config, function(value, name) {


Named function (I can make this change during merge).

phated · 2017-04-22T23:08:39Z

lib/shared/log/toConsole.js


 function toConsole(log, opts) {
+  // Remove previous listeners to enable to call this twice.
+  log.removeAllListeners();


I think this should be more specific to avoid removing other listeners someone else might have attached (we are using a global, after all). I can change this in the merge.

Oh, you are exactly right. Thanks.

phated · 2017-04-22T23:13:17Z

@sttk amazing work, as always. I'll get this merged and make a couple changes I commented about.

sttk · 2017-04-23T00:49:54Z

@phated Thank you for merging!

sttk mentioned this pull request Sep 4, 2016

Restrict config properties that are merged into opts from .gulp.* files #92

Closed

sttk reviewed Sep 5, 2016
View reviewed changes

sttk force-pushed the restrict_config_props branch 2 times, most recently from 92a9107 to 62594e9 Compare October 11, 2016 15:12

sttk force-pushed the restrict_config_props branch from 62594e9 to 8ce1dbe Compare December 4, 2016 08:08

sttk force-pushed the restrict_config_props branch from 8ce1dbe to 3c3ff53 Compare January 7, 2017 12:40

phated requested changes Jan 11, 2017

View reviewed changes

sttk commented Jan 14, 2017

View reviewed changes

Restrict config properties

ead18c6

Modify matters pointed out.

2984265

sttk force-pushed the restrict_config_props branch from 3c3ff53 to 2984265 Compare January 28, 2017 18:15

phated requested changes Apr 19, 2017

View reviewed changes

sttk added 4 commits April 22, 2017 05:22

Rename argument names of load-files

9337fd6

Remove exclude-cli-args

e5bafdd

Remove validations of config files

be189e3

Update toConsole to enable to call twice and remove calls of fancyLog…

4758c86

… in index.js

Remove camelcase from dependencies

23677c9

phated reviewed Apr 22, 2017

View reviewed changes

phated approved these changes Apr 22, 2017

View reviewed changes

phated merged commit 2732c66 into gulpjs:master Apr 22, 2017

phated pushed a commit that referenced this pull request Apr 22, 2017

Update: Restrict config properties (closes #92) (#94)

9753bb9

sttk deleted the restrict_config_props branch April 23, 2017 00:50

phated pushed a commit that referenced this pull request Dec 21, 2017

Update: Restrict config properties (closes #92) (#94)

4cbda4d

phated pushed a commit that referenced this pull request Dec 21, 2017

Update: Restrict config properties (closes #92) (#94)

2fcc2b5

phated pushed a commit that referenced this pull request Dec 21, 2017

Update: Restrict config properties (closes #92) (#94)

649379e

phated pushed a commit that referenced this pull request Dec 21, 2017

Update: Restrict config properties (closes #92) (#94)

a9b4079

phated pushed a commit that referenced this pull request Dec 21, 2017

Update: Restrict config properties (closes #92) (#94)

e859ae9

		@@ -0,0 +1,33 @@
		'use strict';

		var get = require('lodash.get');

Uh oh!

Conversation

sttk commented Sep 4, 2016

Uh oh!

sttk commented Sep 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sttk commented Sep 6, 2016

Uh oh!

phated commented Sep 6, 2016

Uh oh!

sttk commented Sep 6, 2016

Uh oh!

phated commented Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sttk commented Sep 17, 2016

Uh oh!

sttk commented Dec 4, 2016

Uh oh!

phated commented Dec 14, 2016

Uh oh!

phated commented Dec 27, 2016

Uh oh!

phated commented Jan 6, 2017

Uh oh!

sttk commented Jan 7, 2017

Uh oh!

sttk commented Jan 7, 2017

Uh oh!

phated left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sttk Jan 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phated commented Sep 15, 2016 •

edited

Loading

sttk Jan 21, 2017 •

edited

Loading