gulpjs / liftoff

Launch your command line tool with ease.
MIT License
840 stars 52 forks source link

Expose a method to find config files, and move finding module path and package into Liftoff#execute #101

Closed sttk closed 2 years ago

sttk commented 5 years ago

This pr changes as follows to solve gulpjs/gulp-cli#154.

sttk commented 5 years ago

@phated About your first idea above, before the second step, it is needed to take flags.cwd or flags.gulpfile from the config file of the first step and find gulpfile's path (but it is gulp own process and has to be executed out of prepare).

I think the problems to be solved by gulp-cli#154 are following two:

  1. Currently, to load .gulp.*, a (dummy) gulpfile.js in same directory is needed even though the .gulp.* has flags.gulpfile or flags.cwd which changes cwd to another directory.

  2. .gulp.* in the directory changed by --cwd or --gulpfile is used, but .gulp.* in the directory changed by ${initCwd}/.gulp.* is not, even if 1. is solved.

Or 2. is not necessary to solve for gulp?

phated commented 5 years ago

@sttk if we chose to do my 2nd idea, we wouldn't need the 1st. However, it adds a lot of complexity for gulp, that probably isn't really necessary.

sttk commented 5 years ago

@phated extends algorithm of eslint (https://github.com/eslint/eslint/blob/master/lib/config/config-file.js#L422) looks simply merging an array of config files specified by config.extends (but recursively).

Certainly, your 2nd idea will be able to solve the 2nd problem I mentioned above by "extends" field in a .gulp.* in process.cwd().

phated commented 5 years ago

@sttk yeah, that's what I was thinking. We can also bring in some of these changes so only the calculated cwd is loaded and everything else must use extends. We'll probably need to keep the home directory lookup for backwards compatibility though.

phated commented 5 years ago

@sttk should I work on the extends stuff or do you have some ideas about that?

phated commented 5 years ago

@sttk I just pushed a change that removes the launch API. I don't think it's worth us continuing to support that API since this next release will be another major and will probably be released in May (so people had 2 months to upgrade 3.1 patterns). Feel free to rebase and remove those changes from this PR.

sttk commented 5 years ago

I just pushed a change that removes the launch API.

Yeah, I think it's good. I think this pr needs to be changed more. I'll rebase this pr on the latest master branch.

sttk commented 5 years ago

@phated "extends" is not enough for the following case. I'll push the new solution for this pr.

a/
  gulpfile.js
  .gulp.json  // (a)
  b/    <== Initial cwd
    .gulp.json  : { "flags.gulpfile": "../../c/gulpfile.js", "extends": "../../c/.gulp.json" } // (b)
c/
  gulpfile.js
  .gulp.json // (c)

Expected:

  1. Load .gulp.json (b)
  2. Load .gulp.json (c)

Current:

  1. Load .gulp.json (b) // by prepare option
  2. Load .gulp.json (a) // by prepare option

After supporting "extends":

  1. Load .gulp.json (b) // by prepare option
  2. Load .gulp.json (c) // by "extends"
  3. Load .gulp.json (a) // by prepare option
sttk commented 5 years ago

I pushed the new solution for this pr.

The complexity of this issue is due to changing cwd by config files. To solve this issue, js-liftoff have to be able to change env.cwd by the current found config file before finding next config file and to find next config file with the changed env.cwd. However, it depends on application what property in config files to change env.cwd or how many times to change env.cwd by config files. So I added new events and methods to make them possible.

(The test failure of appveyor is seemed to be a installation failure, but it succeeded on my repository.)

phated commented 5 years ago

@sttk I'm not sure why adding extends syntax would cause the a/.gulp.json to be loaded. You might be confusing that with pre-existing behavior (which I was hoping to remove with the same work).

sttk commented 5 years ago

@phated a/.gulp.json is found by fileSearch's finding up. Does this finding up will be removed, too?

-- ADD: fileSearch is called in findConfig, and findConfig is called in buildEnvironment currently, and called in changeEnvPaths in this new commit.

phated commented 5 years ago

@sttk fileSearch is only used when we look up a gulpfile.js, based on my understanding. We use fined to lookup the config files which doesn't do a findup by default (you must specify it as an option). I think we should change the behavior to stop looking for a config file after 1 is found - I don't like the idea that a config could be the result of ./.gulp.json + ~/.gulp.json because it would be a really confusing thing to debug. The ~/.gulp.json should only be loaded if we can't find another .gulp.json

sttk commented 5 years ago

@phated Currently, after looking up gulpfile.js, cwd is changed to the parent directory of gulpfile.js when --cwd is not specified. The clearer logic to resolve cwd and configPath is this. (I forgot some lines, though :cry:). cwd can be changed by this find-up, --cwd, --gulpfile, and config files.

...Now, I think it's better that gulp should look up .gulp.* before looking up gulpfile.* with configNameSearch. Because the first cause of this issue is that renamed gulpfile in cwd cannot be found with .gulp.*. Umm...

phated commented 5 years ago

Now, I think it's better that gulp should look up .gulp. before looking up gulpfile. with configNameSearch. Because the first cause of this issue is that renamed gulpfile in cwd cannot be found with .gulp.*

Yep! That's what I am also thinking. I'm hacking on some ideas for this right now and I think I will have some preliminary code today.