Export prepareConfig #98

sttk · 2019-03-02T15:45:21Z

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

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

phated

After going through this whole PR, I'm wondering if you can separate the Liftoff.prototype.prepareConfig stuff from the fixes in lib/prepare_config.js into separate PRs. The symlink stuff should land as a patch version and then we can work through some of the prepareConfig normalization changes. Thoughts?

README.md

lib/prepare_config.js

phated · 2019-03-02T20:44:47Z

lib/prepare_config.js

+  if (env.configPath) {
+    // resolve symlink if needed
+    if (fs.lstatSync(env.configPath).isSymbolicLink()) {
+      env.configPath = fs.realpathSync(env.configPath);


I was going to ask if this belonged in this file but it was always named weird. We weren't really "preparing" any config, we were just loading explicit requires and loaders.

Does this solve a gulp-cli issue?

I'm not confident about this naming. Certainly this part is just loading opts.requires and loader for configPath. Additionally I want to give the application a chance to change env object here before the loading. Should we provide an empty method (like Liftoff.prototype.prepareConfig) and execute this loading after the method?

Does this solve a gulp-cli issue?

This modification is not for solving any issues. If this symbolic link resolving is needed, this should be applied to configPath by not only cli option but also config files.

As per the comment above, do we want to remove this?

Yes. All tests except a case for symbolic link have passed even without this symbolic link resolving. It's no problem to remove this.

…lve symbolic links for itself.

…eloadModules and registerLoader instead.

sttk · 2019-03-06T15:42:00Z

I've updated this pr.

Remove resolving symbolic link of configPath.
Remove lib/prepare_config.js.
Add Liftoff.prototype.configure, preloadModules and registerLoader instead of prepareConfig.

phated · 2019-03-06T18:05:05Z

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

phated · 2019-03-06T18:31:51Z

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

More changes happening

sttk · 2019-03-07T14:16:45Z

@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 · 2019-03-07T17:58:16Z

@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 · 2019-03-07T21:20:51Z

index.js

  };
+
+  // TODO: Does the "prepare" phase allow us to get rid of this normalization?


@sttk thoughts on this?

flaggedRespawn(flags, argv, [forcedFlags,] execute) regards forcedFlags as execute when forcedFlags is a function. At least, the normalization to avoid forcedFlags not being a function is needed.

@sttk I'm thinking that forcedFlags should only be an option on launch. The execute command can take env and forcedFlags arguments before the callback. I'll make this change.

phated · 2019-03-07T21:24:43Z

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

sttk

@phated I've reviewed and requested some changes.

This prepare/execute separation is elegant!

sttk · 2019-03-09T14:52:46Z

index.js

  };
+
+  // TODO: Does the "prepare" phase allow us to get rid of this normalization?


flaggedRespawn(flags, argv, [forcedFlags,] execute) regards forcedFlags as execute when forcedFlags is a function. At least, the normalization to avoid forcedFlags not being a function is needed.

README.md

test/fixtures/prepare-execute/nodeflags_only.js

phated · 2019-03-09T21:20:55Z

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

sttk

@phated There is one thing I want you to corrent my mistake.
Except this, I think all is good. Excellent!

README.md

phated · 2019-03-10T20:49:08Z

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

phated · 2019-03-10T22:04:29Z

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

sttk · 2019-03-11T12:59:54Z

@phated Thank you very much!

sttk requested a review from phated March 2, 2019 15:45

phated previously requested changes Mar 2, 2019

View reviewed changes

sttk added 2 commits March 6, 2019 23:52

Remove resolving symbolic link of configPath because require reso…

5a13d53

…lve symbolic links for itself.

Remove lib/prepare_config.js, and add Liftoff.prototype.configure, pr…

38abf2b

…eloadModules and registerLoader instead.

add a prepare and execute phase that launch uses

d08e101

phated added 2 commits March 7, 2019 14:13

Add docs

5904b6a

Update tests

89898a8

phated reviewed Mar 7, 2019

View reviewed changes

sttk commented Mar 9, 2019

View reviewed changes

phated added 2 commits March 9, 2019 14:13

Address PR comments

10bd563

Add argv note in callback section

a1bd333

sttk commented Mar 10, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

fix example

f266b8a

phated merged commit 25a40f3 into gulpjs:master Mar 10, 2019

sttk deleted the export_prepare_config branch March 11, 2019 13:00

phated pushed a commit that referenced this pull request Nov 21, 2021

New: Add prepare/execute methods, deprecate launch method (#98)

73e0232

phated pushed a commit that referenced this pull request Nov 21, 2021

New: Add prepare/execute methods, deprecate launch method (#98)

37a5dfc

phated pushed a commit that referenced this pull request Nov 22, 2021

New: Add prepare/execute methods, deprecate launch method (#98)

3c4d041

phated pushed a commit that referenced this pull request Nov 22, 2021

New: Add prepare/execute methods, deprecate launch method (#98)

ad6c47d

phated pushed a commit that referenced this pull request Nov 22, 2021

feat: Add prepare/execute methods, deprecate launch method (#98)

acce613

phated pushed a commit that referenced this pull request Nov 22, 2021

feat: Add prepare/execute methods, deprecate launch method (#98)

cb0623a

phated pushed a commit that referenced this pull request Nov 22, 2021

feat: Add prepare/execute methods, deprecate launch method (#98)

a9d07b1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Export prepareConfig #98

Export prepareConfig #98

sttk commented Mar 2, 2019

phated left a comment

phated Mar 2, 2019

phated Mar 2, 2019

sttk Mar 3, 2019

phated Mar 3, 2019

sttk Mar 3, 2019 •

edited

sttk commented Mar 6, 2019

phated commented Mar 6, 2019

phated commented Mar 6, 2019

sttk commented Mar 7, 2019

phated commented Mar 7, 2019

phated Mar 7, 2019

sttk Mar 9, 2019

phated Mar 9, 2019 •

edited

phated commented Mar 7, 2019

sttk left a comment

sttk Mar 9, 2019

phated commented Mar 9, 2019

sttk left a comment

phated commented Mar 10, 2019

phated commented Mar 10, 2019

sttk commented Mar 11, 2019

		};

		// TODO: Does the "prepare" phase allow us to get rid of this normalization?

Export prepareConfig #98

Export prepareConfig #98

Conversation

sttk commented Mar 2, 2019

phated left a comment

Choose a reason for hiding this comment

phated Mar 2, 2019

Choose a reason for hiding this comment

phated Mar 2, 2019

Choose a reason for hiding this comment

sttk Mar 3, 2019

Choose a reason for hiding this comment

phated Mar 3, 2019

Choose a reason for hiding this comment

sttk Mar 3, 2019 • edited

Choose a reason for hiding this comment

sttk commented Mar 6, 2019

phated commented Mar 6, 2019

phated commented Mar 6, 2019

sttk commented Mar 7, 2019

phated commented Mar 7, 2019

phated Mar 7, 2019

Choose a reason for hiding this comment

sttk Mar 9, 2019

Choose a reason for hiding this comment

phated Mar 9, 2019 • edited

Choose a reason for hiding this comment

phated commented Mar 7, 2019

sttk left a comment

Choose a reason for hiding this comment

sttk Mar 9, 2019

Choose a reason for hiding this comment

phated commented Mar 9, 2019

sttk left a comment

Choose a reason for hiding this comment

phated commented Mar 10, 2019

phated commented Mar 10, 2019

sttk commented Mar 11, 2019

sttk Mar 3, 2019 •

edited

phated Mar 9, 2019 •

edited