gulpjs / liftoff

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

Export prepareConfig #98

Closed sttk closed 5 years ago

sttk commented 5 years ago

Sorry, I forgot to export prepareConfig. This pr modifies Liftoff as follows:

  1. Adds Liftoff.prototype.prepareConfig.
  2. Changes findCwd(opts) to env.cwd for requiring modules specified by env.require. This modification brings no change on behavior but becomes more simple.
  3. Moves resolving symbolic link for env.configPath into prepareConfig. Because it's needed to resolve symbolic link for env.configPath by .gulp.json, too.
sttk commented 5 years ago

I've updated this pr.

phated commented 5 years ago

@sttk these changes are great!! I think I'm going to adjust the API a little bit (hopefully in a backward-compatible way).

phated commented 5 years ago

@sttk I just pushed a change to your fork that splits the launch method into 2 separate methods, a prepare(opts, fn) and an execute(env, fn). Then launch uses them to retain the previous behavior. However, in gulp-cli, we can use the methods independently like:

var cli = new Liftoff()
cli.prepare(opts, function(env) {
  var cfgLoadOrder = ['home', 'cwd'];
  var cfg = loadConfigFiles(env.configFiles['.gulp'], cfgLoadOrder);
  opts = mergeConfigToCliFlags(opts, cfg);
  env = mergeConfigToEnvFlags(env, cfg);
  env.configProps = cfg;

  cli.execute(env, function() {
     // post-launch stuff
  });
});

What do you think about these changes? If they seem good, we can document them and refactor the tests. (I'm also thinking about making these APIs Promise-returning in the future so they can be used with async/await)

sttk commented 5 years ago

@phated Interesting! I think those changes are good!

I'm also thinking about making these APIs Promise-returning in the future so they can be used with async/await

That code will become as follows?

const cli = new Liftoff()
async function run() {
  const env = await cli.promisifiedPrepare(opts)
  // process configFiles
  await cli.promisifiedExecute(env)
  // post-launch stuff
}
run()

And will you drop support of older nodejs versions than v7.6 in the near future?

phated commented 5 years ago

@sttk perfect, I'll finish up the rest of this today and then you can do a full review!

As for the promise stuff, that will probably be in the next major release after we get some of these little things fixed, but your proposed syntax would be correct (the methods likely won't be renamed though). I'm still trying to determine when the older node versions will be dropped because gulp has a subset of users still on old versions.

phated commented 5 years ago

@sttk I've updated the docs and the tests for these changes. Please review and let me know if anything should be changed.

Also, can you make sure I didn't break anything in the gulp-cli test suite with these changes?

phated commented 5 years ago

@sttk I've addressed all of your comments and made execute take the forcedFlags as the 2nd argument. Let me know if these changes look good and I can merge and make a release.

phated commented 5 years ago

💥 🎉 :shipit: @sttk thank you so much for getting this PR started. I will get it released shortly.

phated commented 5 years ago

3.1.0 published!! I'm excited to get this into gulp-cli and fix up some of our outstanding issues!

sttk commented 5 years ago

@phated Thank you very much!