-
-
Notifications
You must be signed in to change notification settings - Fork 106
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
Add initial cwd to directories for searching config files #154
Add initial cwd to directories for searching config files #154
Conversation
@sttk I assume this is going to be changed with the new liftoff stuff? |
@phated Yes. I think so, too. In addition, I think we can include a change for |
In separate PRs 😛 |
07fb7c0
to
be9a912
Compare
dab1404
to
a4236f2
Compare
Sorry. I made a mistake on pushing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sttk Great work, I love how this PR got cleaned up with the Liftoff changes! I just had one question for you.
@@ -89,7 +93,8 @@ function run() { | |||
require: opts.require, | |||
completion: opts.completion, | |||
}, function(env) { | |||
var cfgLoadOrder = ['home', 'cwd']; | |||
|
|||
var cfgLoadOrder = ['home', 'cwd', 'initCwd']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sttk I might be wrong but shouldn't the order be ['home', 'initCwd', 'cwd']
? I think that initCwd
is a fallback if we can't find one inside cwd
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phated Perhaps you are right, but one test case fails by this because of the bug that flags.gulpfile
does not change cwd. We should fix the bug before this.
feb590d
to
4d8248d
Compare
test/config-description.js
Outdated
stdout = skipLines(eraseTime(stdout), 1); | ||
|
||
expect(headLines(stdout, 1)).toEqual(headLines(expected0, 1)); | ||
expect(skipLines(stdout, 1)).toEqual(skipLines(expected3, 1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior of this seems extremely awkward. When using --cwd
, I wouldn't expect the flags.gulpfile
to be picking up a gulpfile where I ran my command - I'd expect it to run a gulpfile where my --cwd
was pointing.
test/config-flags-gulpfile.js
Outdated
'\n\t--gulpfile', function(done) { | ||
// This test case fails because of a bug that `flags.gullpfile` does not | ||
// change process.cwd(). | ||
it.skip('Should use config file here and use gulpfile specified in config file', function(done) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sttk I just went through this and I think it's too big of issue to ship these changes without having this working correctly.
I think it's a fundamental flaw in how we added config files to Liftoff, because they don't affect it's configBase, etc lookups when we rename the gulpfile.
I'm not sure how to solve this.
@sttk I'm continuing to dig into this and I think the behavior is even more incorrect when I'm experimenting with |
@sttk I've also found that when using a |
4d8248d
to
de48b19
Compare
@sttk I've just force-pushed a rebase on the lastest master plus my changes where I think our behavior is incorrect. I've tried to note in comments how things aren't working correctly or the temporary solutions to some problems exist. Please let me know what you think of all of this. |
@phated This issue is very difficult. Please give me some time to think. |
@phated Does what your changes suggest that if a config file in It means that flags by config files in |
If so, I have two ideas to fix this issue: (The following codes are not actual but an example.)
var cli = new Liftoff({ ... });
var foundInInitCwd = fined({ path: '.', name: '.gulp', extensions: cli.extensions });
var cfg = { flags: {} };
if (foundInInitCwd) {
cfg = require(foundInInitCwd.path);
opts = mergeConfigToCliFlags(opts, cfg);
if (!opts.gulpfile) { opts.gulpfile = cfg.flags.gulpfile; }
if (!opts.cwd) { opts.cwd = cfg.flags.cwd || cfg.flags.gulpfile; }
}
cli.prepare({ cwd: opts.cwd, configPath: opts.gulpfile, ... }, function(env) {
var cfgLoadOrder = ['home'];
if (env.configFiles['.gulp']['cwd'].path !== foundInInitCwd.path) { cfgLoadOrder.push('cwd'); }
var cfg = loadConfigFiles(env.configFiles['.gulp'], cfgLoadOrder);
opts = mergeConfigToCliFlags(opts, cfg);
env = mergeConfigToEnvFlags(env, cfg, opts);
...
cli.execute(env, handleArguments);
});
var opts = { ... };
var env = {};
var cli = new Liftoff({
...
configFiles: {
'.gulp': {
initCwd: {
order: 1,
path: opts.cwd || process.env.INIT_CWD,
onFound: loadAndMergeConfig,
},
cwd: {
order: 2,
path: env.cwd
onFound: loadAndMergeConfig,
},
home: {
order: 3,
path: '~',
onFound: loadAndMergeConfig,
},
},
},
});
cli.prepare({ cwd: opts.cwd, configPath: opts.gulpfile, ... }, function(env) {
...
cli.execute(env, handleArguments);
});
function loadAndMergeConfigs(name, path) {
var cfg = require(path);
opts = mergeConfigToCliFlags(opts, cfg);
env = mergeConfigToEnvFlags(env, cfg, opts);
} |
@sttk I think the issue is actually that too much filesystem traversal is being made in The reason I think this is that the Lines 43 to 54 in 1b80d67
I believe that is what you were trying to achieve by adding |
Rather, that moving may be necessary because there is a case that gulp cannot be run only by changing cwd and gulpfile. It's needed to set
|
Furthermore, the current gulp with
|
I've add
initCwd
entry to the last ofopts.configFiles
.This PR is for the issue #146 and changes gulp's behaviors about using config file and selecting gulpfile.
gulpfile.*
.flags.gulpfile
property.In the situation above, gulp after this modification uses the config file in current dir and uses gulpfile specified in the config file, but changes cwd to the ancestor dir.
Before this modification, gulp ignores this config file, and finds up a gulpfile in the ancestor dir, changes cwd to the dir and uses the gulpfile.