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

New: Support flags.require config option (closes #108, closes #167) #183

Merged
merged 4 commits into from
Mar 21, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ Supported configurations properties:
| flags.gulpfile | Set a default gulpfile |
| flags.silent | Silence logging by default |
| flags.series | Run tasks given on the CLI in series (the default is parallel) |
| flags.require | An array of modules to require 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

Expand Down Expand Up @@ -147,7 +148,7 @@ __Some flags only work with gulp 4 and will be ignored when invoked against gulp
<tr>
<td>--cwd [path]</td>
<td></td>
<td>Manually set the CWD. The search for the gulpfile, as well as the relativity of all requires will be from here.</td>
<td>Manually set the CWD. The search for the gulpfile, as well as the relativity of all requires (including the `--require` flag) will be from here.</td>
</tr>
<tr>
<td>--verify [path (optional)]</td>
Expand Down
4 changes: 4 additions & 0 deletions lib/shared/config/env-flags.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ var copyProps = require('copy-props');
var toFrom = {
configPath: 'flags.gulpfile',
configBase: 'flags.gulpfile',
require: 'flags.require',
};

function mergeConfigToEnvFlags(env, config) {
Expand All @@ -16,6 +17,9 @@ function convert(configInfo, envInfo) {
if (envInfo.keyChain === 'configBase') {
return path.dirname(configInfo.value);
}
if (envInfo.keyChain === 'require') {
return [].concat(envInfo.value, configInfo.value);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This code should be moved to load-files.js, which is executed by each config file.
env-flags.js is executed only once for copying configs to env after merging config files.

Take notice that if a value of flags.require is relative path, it is treated as relative to cwd when requiring. So this value needs to be converted to an absolute path when loading like flags.gulpfile.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I knew I was probably doing something wrong. I'll add a test for that too.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sttk I am looking at this now and I don't think this belongs in load-files.js because that would combine the config from cwd and home config files. Everything we've done until now has been that cwd configuration overrides any configuration in home.

I think we should continue that for this configuration, because it would be hard to track where the require was coming from. Also, it'd be the only option that combines a field from both locations and I'd like to keep everything consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sttk currently, when using --cwd and --require on the command-line, you have to be aware that your file will be required relative to cwd, is that not the behavior we want to reflect when using the .gulp.json file? Or do we need to fix the --require flag as well?

I'm not sure what behavior I would expect from this combination, so I don't know how we should change (if at all).

Copy link
Contributor

@sttk sttk Mar 15, 2019

Choose a reason for hiding this comment

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

cwd configuration overrides any configuration in home.

The above code looks not overriding but appending require configuration. With no conversion, load-files.js overrides configurations. So if overriding, only normalizing configInfo.value to array is needed. If overriding, that concatenation is not needed. (Though normalization of configInfo.value to array is needed.)

Copy link
Contributor

Choose a reason for hiding this comment

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

But I think both two cases (overriding and appending) can be requested. For example, a TS user uses ts-node ususally so writes requiring it in ~/.gulp.json, and writes more loaders in (project-dir)/.gulp.json by project, but he don't want to use ts-node in only one project.

What do you think about adding one more format to choice overriding or appending like { "flags.require": { "path": "module-name", "append": true } } ?

Copy link
Contributor

Choose a reason for hiding this comment

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

About a relative path, I think better that a relative path written in ~/.gulp.* is relative to home dir because it is looked and can be used by all project, and directory structures of projects are different.

But I notice that what I'm saying can be solved by specifying an absolute path. And certainly, what I'm saying ignores --cwd/gulpfile options. Umm...

Copy link
Member Author

Choose a reason for hiding this comment

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

@sttk Joining flags.require from cwd and home will break people that pull the project from GitHub (for example, your ts-node example would break for me if I pulled that down and didn't have the ts-node in my home .gulp.js file) - we'd want to have all the flags.require values in the cwd .gulp.js file.

I think I need to keep this array concat because I want to join the --require flags and the flags.require property. This should be the behavior for anything that takes multiple argument values.

What do you think about adding one more format to choice overriding or appending like { "flags.require": { "path": "module-name", "append": true } } ?

I don't want to max the options more complex like you suggested.

About a relative path, I think better that a relative path written in ~/.gulp.* is relative to home dir because it is looked and can be used by all project, and directory structures of projects are different.
But I notice that what I'm saying can be solved by specifying an absolute path. And certainly, what I'm saying ignores --cwd/gulpfile options. Umm...

I think we should just note that flags.require will be resolved to cwd unless it's a node module or absolute path. I can make that change to the docs.

Copy link
Contributor

@sttk sttk Mar 21, 2019

Choose a reason for hiding this comment

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

Joining flags.require from cwd and home will break people that pull the project from GitHub

Certainly. it might be rare to use flags.require in home.

I think I need to keep this array concat because I want to join the --require flags and the flags.require property.

I got it. I think this should be so, too.

I don't want to max the options more complex like you suggested.

I got it.

I think we should just note that flags.require will be resolved to cwd unless it's a node module or absolute path. I can make that change to the docs.

Thanks for writing docs. I checked it.

return configInfo.value;
}

Expand Down
109 changes: 109 additions & 0 deletions test/config-flags-require.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
'use strict';

var expect = require('expect');

var path = require('path');
var fixturesDir = path.join(__dirname, 'fixtures/config');

var headLines = require('gulp-test-tools').headLines;
phated marked this conversation as resolved.
Show resolved Hide resolved
var eraseTime = require('gulp-test-tools').eraseTime;
var runner = require('gulp-test-tools').gulpRunner().basedir(fixturesDir);

describe('config: flags.require', function() {

it('Should configure with an array in a .gulp.* file', function(done) {
runner
.chdir('flags/require/array')
.gulp()
.run(cb);

function cb(err, stdout, stderr) {
expect(err).toEqual(null);
expect(stderr).toEqual('');

var requiring1 = eraseTime(headLines(stdout, 1));
expect(requiring1).toEqual('Requiring external module ./preload_one');
var requiring2 = eraseTime(headLines(stdout, 1, 1));
expect(requiring2).toEqual('Requiring external module ./preload_two');
var preload1 = eraseTime(headLines(stdout, 1, 4));
expect(preload1).toEqual('preload one!');
var preload2 = eraseTime(headLines(stdout, 1, 5));
expect(preload2).toEqual('preload two!');
done(err);
}
});

it('Should configure with a string in a .gulp.* file', function(done) {
runner
.chdir('flags/require/string')
.gulp()
.run(cb);

function cb(err, stdout, stderr) {
expect(err).toEqual(null);
expect(stderr).toEqual('');
var requiring = eraseTime(headLines(stdout, 1));
expect(requiring).toEqual('Requiring external module ./preload');
var preload1 = eraseTime(headLines(stdout, 1, 3));
expect(preload1).toEqual('hello preload!');
done(err);
}
});

it('Combines --require flag with .gulp.* file flags.require', function(done) {
runner
.chdir('flags/require/join-flags')
.gulp('--require ./preload_one')
.run(cb);

function cb(err, stdout, stderr) {
expect(err).toEqual(null);
expect(stderr).toEqual('');

var requiring1 = eraseTime(headLines(stdout, 1));
expect(requiring1).toEqual('Requiring external module ./preload_one');
var requiring2 = eraseTime(headLines(stdout, 1, 1));
expect(requiring2).toEqual('Requiring external module ./preload_two');
var preload1 = eraseTime(headLines(stdout, 1, 4));
expect(preload1).toEqual('preload one!');
var preload2 = eraseTime(headLines(stdout, 1, 5));
expect(preload2).toEqual('preload two!');
done(err);
}
});

it('resolves relative requires against cwd', function(done) {
runner
.gulp('--cwd flags/require/with-cwd')
.run(cb);

function cb(err, stdout, stderr) {
expect(err).toEqual(null);
expect(stderr).toEqual('');
var requiring = eraseTime(headLines(stdout, 1));
expect(requiring).toEqual('Requiring external module ../preload');
var preload1 = eraseTime(headLines(stdout, 1, 4));
expect(preload1).toEqual('hello preload!');
done(err);
}
});

it('works with absolute paths, ignoring cwd', function(done) {
runner
.gulp('--cwd flags/require/with-absolute')
.run(cb);

function cb(err, stdout, stderr) {
expect(err).toEqual(null);
expect(stderr).toEqual('');

var absolute = path.join(__dirname, './fixtures/config/flags/require/preload');
var requiring = eraseTime(headLines(stdout, 1));
expect(requiring).toEqual('Requiring external module ' + absolute);
var preload1 = eraseTime(headLines(stdout, 1, 4));
expect(preload1).toEqual('hello preload!');
done(err);
}
});
});

8 changes: 8 additions & 0 deletions test/fixtures/config/flags/require/array/.gulp.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"flags": {
"require": [
"./preload_one",
"./preload_two"
]
}
}
9 changes: 9 additions & 0 deletions test/fixtures/config/flags/require/array/gulpfile.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
'use strict';

var gulp = require('gulp');

gulp.task('default', function(done) {
console.log(global.preload_one);
console.log(global.preload_two);
done();
});
1 change: 1 addition & 0 deletions test/fixtures/config/flags/require/array/preload_one.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
global.preload_one = 'preload one!';
1 change: 1 addition & 0 deletions test/fixtures/config/flags/require/array/preload_two.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
global.preload_two = 'preload two!';
7 changes: 7 additions & 0 deletions test/fixtures/config/flags/require/join-flags/.gulp.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"flags": {
"require": [
"./preload_two"
]
}
}
9 changes: 9 additions & 0 deletions test/fixtures/config/flags/require/join-flags/gulpfile.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
'use strict';

var gulp = require('gulp');

gulp.task('default', function(done) {
console.log(global.preload_one);
console.log(global.preload_two);
done();
});
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
global.preload_one = 'preload one!';
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
global.preload_two = 'preload two!';
1 change: 1 addition & 0 deletions test/fixtures/config/flags/require/preload.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
global.preload = 'hello preload!';
5 changes: 5 additions & 0 deletions test/fixtures/config/flags/require/string/.gulp.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"flags": {
"require": "./preload"
}
}
8 changes: 8 additions & 0 deletions test/fixtures/config/flags/require/string/gulpfile.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
'use strict';

var gulp = require('gulp');

gulp.task('default', function(done) {
console.log(global.preload);
done();
});
1 change: 1 addition & 0 deletions test/fixtures/config/flags/require/string/preload.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
global.preload = 'hello preload!';
7 changes: 7 additions & 0 deletions test/fixtures/config/flags/require/with-absolute/.gulp.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
var path = require('path');

module.exports = {
flags: {
require: path.join(__dirname, '../preload'),
},
};
8 changes: 8 additions & 0 deletions test/fixtures/config/flags/require/with-absolute/gulpfile.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
'use strict';

var gulp = require('gulp');

gulp.task('default', function(done) {
console.log(global.preload);
done();
});
5 changes: 5 additions & 0 deletions test/fixtures/config/flags/require/with-cwd/.gulp.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"flags": {
"require": "../preload"
}
}
8 changes: 8 additions & 0 deletions test/fixtures/config/flags/require/with-cwd/gulpfile.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
'use strict';

var gulp = require('gulp');

gulp.task('default', function(done) {
console.log(global.preload);
done();
});
4 changes: 4 additions & 0 deletions test/fixtures/test-module-2.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
console.log('inside test module 2');
exports = function() {
console.log('inside test module function 2');
};
26 changes: 26 additions & 0 deletions test/flags-require.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,32 @@ describe('flag: --require', function() {
}
});

it('can require multiple modules before running gulpfile', function(done) {
runner({ verbose: false })
.gulp('--require ../test-module.js', '--require ../test-module-2.js', '--cwd ./test/fixtures/gulpfiles')
.run(cb);

function cb(err, stdout, stderr) {
expect(err).toEqual(null);
expect(stderr).toEqual('');
var insideLog = headLines(stdout, 1);
expect(insideLog).toEqual('inside test module');

var requireLog = eraseTime(headLines(stdout, 1, 1));
expect(requireLog).toEqual(
'Requiring external module ../test-module.js');

var insideLog2 = headLines(stdout, 1, 2);
expect(insideLog2).toEqual('inside test module 2');

var requireLog2 = eraseTime(headLines(stdout, 1, 3));
expect(requireLog2).toEqual(
'Requiring external module ../test-module-2.js');

done(err);
}
});

it('warns if module doesn\'t exist', function(done) {
runner({ verbose: false })
.gulp('--require ./null-module.js', '--cwd ./test/fixtures/gulpfiles')
Expand Down