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

Use theming-log for logging. #161

Closed
wants to merge 10 commits into from
2 changes: 1 addition & 1 deletion .eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"extends": "gulp",
"rules": {
"max-len": [1, 130],
"max-statements": [1, 40],
"max-statements": [1, 60],
"no-console": "off"
}
}
6 changes: 6 additions & 0 deletions .github/workflows/dev.yml
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,12 @@ jobs:
# Run test without coverage because a behavior about esm is different with nyc or not
run: npm test

- name: Run tests with coloring
run: npx mocha test/lib/theme.js
env:
FORCE_COLOR: 2
CI: ""

coveralls:
needs: test
name: Finish up
Expand Down
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
*.log
node_modules
!test/fixtures/errors/bad-gulp-version/node_modules/
!test/fixtures/config/theming/error/badGulpVersion/node_modules/
build
*.node
components
Expand Down
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,6 @@ Supported configurations properties:

| Property | Description |
|--------------------|-------------|
| description | Top-level description of the project/gulpfile (Replaces "Tasks for ~/path/of/gulpfile.js") |
| flags.continue | Continue execution of tasks upon failure by default. |
| flags.compactTasks | Reduce the output of task dependency tree by default. |
| flags.tasksDepth | Set default depth of task dependency tree. |
Expand All @@ -116,6 +115,8 @@ Supported configurations properties:
| flags.series | Run tasks given on the CLI in series (the default is parallel) |
| flags.preload | An array of modules to preload before running the gulpfile. Any relative paths will be resolved against the `--cwd` directory (if you don't want that behavior, use absolute paths) |
| flags.nodeFlags | An array of flags used to forcibly respawn the process upon startup. For example, if you always want your gulpfiles to run in node's harmony mode, you can set `--harmony` here |
| log.msgs.* | Configure log messages. |
| log.theme.* | Configure log theme. |

## Flags

Expand Down
91 changes: 34 additions & 57 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,23 @@
var fs = require('fs');
var path = require('path');
var log = require('gulplog');

var Liftoff = require('liftoff');
var interpret = require('interpret');
var v8flags = require('v8flags');
var findRange = require('semver-greatest-satisfied-range');
var chalk = require('chalk');

var exit = require('./lib/shared/exit');
var tildify = require('./lib/shared/tildify');
var makeTitle = require('./lib/shared/make-title');
var parser = require('./lib/shared/options/parser');
var makeHelp = require('./lib/shared/options/make-help');
var completion = require('./lib/shared/completion');
var cliVersion = require('./package.json').version;
var toConsole = require('./lib/shared/log/to-console');
var msgs = require('./lib/shared/log/messages');

var mergeProjectAndUserHomeConfigs = require('./lib/shared/config/merge-configs');
var overrideEnvFlagsByConfigAndCliOpts = require('./lib/shared/config/env-flags');
var overrideEnvByConfigAndCliOpts = require('./lib/shared/config/env-config');

// Get supported ranges
var ranges = fs.readdirSync(path.join(__dirname, '/lib/versioned/'));
Expand Down Expand Up @@ -52,25 +53,24 @@ var cli = new Liftoff({
},
});

var opts = parser.argv;
var opts = {};
var optsErr;
try {
opts = parser.argv;
} catch (e) {
optsErr = e;
}

cli.on('preload:before', function(name) {
log.info('Preloading external module:', chalk.magenta(name));
log.info(msgs.info.preloadBefore, name);
});

cli.on('preload:success', function(name) {
log.info('Preloaded external module:', chalk.magenta(name));
log.info(msgs.info.preloadSuccess, name);
});

cli.on('preload:failure', function(name, error) {
log.warn(
chalk.yellow('Failed to preload external module:'),
chalk.magenta(name)
);
/* istanbul ignore else */
if (error) {
log.warn(chalk.yellow(error.toString()));
}
log.warn(msgs.warn.preloadFailure, name, Boolean(error), error.toString());
Copy link
Member

Choose a reason for hiding this comment

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

I don't like combining the preload message failure and the error printing. Can we keep them separate?

});

cli.on('loader:success', function(name) {
Expand All @@ -79,26 +79,16 @@ cli.on('loader:success', function(name) {
// However, we don't want to show the mjs-stub loader in the logs
/* istanbul ignore else */
if (path.basename(name, '.js') !== 'mjs-stub') {
log.info('Loaded external module:', chalk.magenta(name));
log.info(msgs.info.loaderSuccess, name);
}
});

cli.on('loader:failure', function(name, error) {
log.warn(
chalk.yellow('Failed to load external module:'),
chalk.magenta(name)
);
/* istanbul ignore else */
if (error) {
log.warn(chalk.yellow(error.toString()));
}
log.warn(msgs.warn.loaderFailure, name, Boolean(error), error.toString());
Copy link
Member

Choose a reason for hiding this comment

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

I don't like combining the preload message failure and the error printing. Can we keep them separate?

});

cli.on('respawn', function(flags, child) {
var nodeFlags = chalk.magenta(flags.join(', '));
var pid = chalk.magenta(child.pid);
log.info('Node flags detected:', nodeFlags);
log.info('Respawned to PID:', pid);
log.info(msgs.info.respawn, flags.join(', '), child.pid);
Copy link
Member

Choose a reason for hiding this comment

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

These should not be combined, one message is about detecting node flags and one if about the respawn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that users configuring this would consider these two lines as one.
Additionally, in this way, it is also possible to either output only one line or not output both lines.

});

function run() {
Expand All @@ -114,7 +104,7 @@ module.exports = run;

function onPrepare(env) {
var cfg = mergeProjectAndUserHomeConfigs(env);
env = overrideEnvFlagsByConfigAndCliOpts(env, cfg, opts);
env = overrideEnvByConfigAndCliOpts(env, cfg, opts);

// Set up event listeners for logging again after configuring.
toConsole(log, env.config.flags);
Expand All @@ -132,8 +122,13 @@ function onExecute(env) {
process.env.UNDERTAKER_SETTLE = 'true';
}

if (optsErr) {
log.error(msgs.error.failToParseCliOpts, optsErr.message);
makeHelp(parser).showHelp(console.error);
exit(1);
}
Comment on lines +125 to +129
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part is needed against an error process of yargs for invalid option.
For example, gulp -f causes a yargs error because -f/—gulpfile requires an argument. By default, yargs outputs the error message Not enough arguments following: f, outputs help messages, and exits the program.
To control this behavior and configure these messages, I added this part and another part in lib/shared/options/parser.js.

if (env.config.flags.help) {
parser.showHelp(console.log);
makeHelp(parser).showHelp(console.log);
exit(0);
}

Expand All @@ -145,56 +140,38 @@ function onExecute(env) {
}

if (!env.modulePath) {
/* istanbul ignore next */
var missingNodeModules =
fs.existsSync(path.join(env.cwd, 'package.json'))
&& !fs.existsSync(path.join(env.cwd, 'node_modules'));

/* istanbul ignore next */
var missingGulpMessage =
missingNodeModules
? 'Local modules not found in'
: 'Local gulp not found in';
log.error(
chalk.red(missingGulpMessage),
chalk.magenta(tildify(env.cwd))
);
var hasYarn = fs.existsSync(path.join(env.cwd, 'yarn.lock'));
/* istanbul ignore next */
var installCommand =
missingNodeModules
? hasYarn
? 'yarn install'
: 'npm install'
: hasYarn
? 'yarn add gulp'
: 'npm install gulp';
log.error(chalk.red('Try running: ' + installCommand));
var hasNpm = !hasYarn;

if (missingNodeModules) {
log.error(msgs.error.nodeModulesNotFound, tildify(env.cwd), hasYarn, hasNpm);
} else {
log.error(msgs.error.gulpNotFound, tildify(env.cwd), hasYarn, hasNpm);
Comment on lines +151 to +153
Copy link
Member

Choose a reason for hiding this comment

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

These seem strange to me right now. I need to think more.

}
exit(1);
}

if (!env.configPath) {
log.error(chalk.red('No gulpfile found'));
log.error(msgs.error.gulpfileNotFound);
exit(1);
}

// Chdir before requiring gulpfile to make sure
// we let them chdir as needed
if (process.cwd() !== env.cwd) {
process.chdir(env.cwd);
log.info(
'Working directory changed to',
chalk.magenta(tildify(env.cwd))
);
log.info(msgs.info.cwdChanged, tildify(env.cwd));
}

// Find the correct CLI version to run
var range = findRange(env.modulePackage.version, ranges);

if (!range) {
log.error(
chalk.red('Unsupported gulp version', env.modulePackage.version)
);
log.error(msgs.error.badGulpVersion, env.modulePackage.version);
exit(1);
}

Expand Down
12 changes: 6 additions & 6 deletions lib/shared/completion.js
Copy link
Member

Choose a reason for hiding this comment

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

This file is for providing shell completions, we probably don't need to use formatting in here at all.

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 also aim to enable multilingualization support with this change. I want to make it possible to configure all messages generated by gulp-cli.

Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,21 @@

var fs = require('fs');
var path = require('path');
var format = require('theming-log').format;

var theme = require('./log/theme');
var msgs = require('./log/messages');

module.exports = function(name) {
if (typeof name !== 'string') {
throw new Error('Missing completion type');
throw new Error(format(theme, msgs.error.noCompletionType));
Copy link
Member

Choose a reason for hiding this comment

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

Why do we format the message for a thrown error?

}
var file = path.join(__dirname, '../../completion', name);
try {
console.log(fs.readFileSync(file, 'utf8'));
process.exit(0);
} catch (err) {
console.log(
'echo "gulp autocompletion rules for',
'\'' + name + '\'',
'not found"'
);
console.log(format(theme, msgs.error.unknownCompletionType, name));
Copy link
Member

Choose a reason for hiding this comment

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

Since this uses console.log, we probably don't need to allow it to be configured.

process.exit(5);
}
};
20 changes: 16 additions & 4 deletions lib/shared/config/env-flags.js → lib/shared/config/env-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,17 @@ var copyProps = require('copy-props');

var mergeCliOpts = require('./cli-flags');

var theme = require('../log/theme');
var msgs = require('../log/messages');

var toEnvFromConfig = {
configPath: 'flags.gulpfile',
configBase: 'flags.gulpfile',
preload: 'flags.preload',
nodeFlags: 'flags.nodeFlags',
};

function overrideEnvFlags(env, config, cliOpts) {
function overrideEnvConfig(env, config, cliOpts) {
cliOpts = mergeCliOpts(cliOpts, config);

// This must reverse because `flags.gulpfile` determines 2 different properties
Expand All @@ -21,9 +24,18 @@ function overrideEnvFlags(env, config, cliOpts) {

env.config = {
flags: cliOpts,
log: {
theme: theme,
msgs: msgs,
},
};
if (config.description) {
env.config.description = config.description;
if (config.log) {
if (config.log.theme) {
copyProps(config.log.theme, env.config.log.theme);
}
if (config.log.msgs) {
copyProps(config.log.msgs, env.config.log.msgs);
}
}
return env

Expand Down Expand Up @@ -53,4 +65,4 @@ function overrideEnvFlags(env, config, cliOpts) {
}
}

module.exports = overrideEnvFlags;
module.exports = overrideEnvConfig;
1 change: 1 addition & 0 deletions lib/shared/config/merge-configs.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ var path = require('path');

function mergeConfigs(env) {
var cfg = {};
/* istanbul ignore if */
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For increasing coverage. It's impossible to test a config file in user home.

if (env.configFiles.userHome) {
copyConfig(env.config.userHome, cfg, env.configFiles.userHome);
}
Expand Down