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

phated · 2019-03-13T20:11:22Z

@sttk please review

sttk

@phated This is great! It slips my mind to need to concat flags.require in multiple .gulp.*.

Please change for my two comments.

sttk · 2019-03-14T15:14:11Z

lib/shared/config/env-flags.js

@@ -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);
+  }


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.

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

@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.

@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).

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.)

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 } } ?

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...

@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.

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.

test/config-flags-require.js

phated · 2019-03-19T03:44:33Z

@sttk I added docs and tests based on our discussions. Please review again and hopefully we can merge this.

phated · 2019-03-20T18:44:22Z

@sttk you are probably busy, but could you review this so we can land it?

sttk · 2019-03-20T20:20:46Z

@phated Sorry for the delay. I'll review this today. (It's a public holiday in japan today)

phated · 2019-03-20T23:22:28Z

Thanks! Sorry to interrupt your holiday 😭

sttk

@phated Don't worry. What I wanted to say is I have time to review today.

I think all is good and no problem. I approve this.

sttk · 2019-03-21T01:40:58Z

lib/shared/config/env-flags.js

@@ -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);
+  }


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.

phated · 2019-03-21T22:24:08Z

@sttk thanks for reviewing!

phated requested a review from sttk March 13, 2019 20:11

terinjokes approved these changes Mar 13, 2019

View reviewed changes

phated mentioned this pull request Mar 13, 2019

Suppress logging when --tasks-json flag is specified #159

Merged

sttk suggested changes Mar 14, 2019

View reviewed changes

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

607e105

phated force-pushed the flags-require branch from cb5b041 to 607e105 Compare March 19, 2019 03:01

phated added 3 commits March 18, 2019 20:04

Update docs to reflect require against cwd

42b9d8b

Add tests for absolute and relative flags.require

554c6e1

Add test for --require twice

860822d

phated requested a review from sttk March 19, 2019 03:44

sttk approved these changes Mar 21, 2019

View reviewed changes

sttk mentioned this pull request Mar 21, 2019

Expose flags.cwd as config option #109

Open

phated merged commit 33e14d7 into master Mar 21, 2019

phated deleted the flags-require branch March 21, 2019 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

phated commented Mar 13, 2019

sttk left a comment

sttk Mar 14, 2019

phated Mar 14, 2019

phated Mar 14, 2019

phated Mar 14, 2019

sttk Mar 15, 2019 •

edited

sttk Mar 15, 2019

sttk Mar 15, 2019

phated Mar 18, 2019

sttk Mar 21, 2019 •

edited

phated commented Mar 19, 2019

phated commented Mar 20, 2019

sttk commented Mar 20, 2019

phated commented Mar 20, 2019

sttk left a comment

sttk Mar 21, 2019 •

edited

phated commented Mar 21, 2019

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

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

Conversation

phated commented Mar 13, 2019

sttk left a comment

Choose a reason for hiding this comment

sttk Mar 14, 2019

Choose a reason for hiding this comment

phated Mar 14, 2019

Choose a reason for hiding this comment

phated Mar 14, 2019

Choose a reason for hiding this comment

phated Mar 14, 2019

Choose a reason for hiding this comment

sttk Mar 15, 2019 • edited

Choose a reason for hiding this comment

sttk Mar 15, 2019

Choose a reason for hiding this comment

sttk Mar 15, 2019

Choose a reason for hiding this comment

phated Mar 18, 2019

Choose a reason for hiding this comment

sttk Mar 21, 2019 • edited

Choose a reason for hiding this comment

phated commented Mar 19, 2019

phated commented Mar 20, 2019

sttk commented Mar 20, 2019

phated commented Mar 20, 2019

sttk left a comment

Choose a reason for hiding this comment

sttk Mar 21, 2019 • edited

Choose a reason for hiding this comment

phated commented Mar 21, 2019

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

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

sttk Mar 15, 2019 •

edited

sttk Mar 21, 2019 •

edited

sttk Mar 21, 2019 •

edited