gulpjs / gulp

A toolkit to automate & enhance your workflow
https://gulpjs.com
MIT License
33.02k stars 4.23k forks source link

gulp.series() could be friendlier to run inside a function #2116

Closed gauntface closed 6 years ago

gauntface commented 6 years ago

There are a few scenarios where I've been wanting to write a task similar to:

gulp.task('prod', () => {
    process.env.NODE_ENV = 'production';

    return gulp.series([
        'build'
        'serviceWorker',
    ]);
});

This doesn't work because gulp.series() returns a function, and the execution of a task is expected to return a promise or a stream.

So executing gulp.series, like so:

gulp.task('prod', () => {
    process.env.NODE_ENV = 'production';

    return gulp.series([
        'build'
        'serviceWorker',
    ])();
});

Running the prod task, it'll correctly run the series tasks, but at the end it'll print:

[11:10:12] The following tasks did not complete: prod [11:10:12] Did you forget to signal async completion?

In the end I need to write:

gulp.task('prod', (done) => {
    process.env.NODE_ENV = 'production';

    return gulp.series([
        'build'
        'serviceWorker',
    ])(done);
});

This works, but it would have been amazing if:

  1. Returning a function as a result of a gulp task would cause the function to be run. This means developers could do:
    gulp.task('example', () => {
        // TODO: Do stuff here
        return gulp.series([....]);  
    });
  2. The return type of gulp.series()() was suitable for gulp.task to be happy once it's completed (i.e. return a promise or stream or gulp.task expect the result).
phated commented 6 years ago

@gauntface hmmm. The general idea of these APIs is to compose functions at the top level. The pattern you showed is something that can be done due to the implementation but it's not a recommended pattern (thus looks bad). If we added your suggestion, I feel like many more people would be adding extra function wrappers into their programs (like gulp.task('example', () => gulp.series(...));) which we really don't want.

I think most of these scenarios go away when people utilize flags and task metadata). (And named functions in the cases where they are dodging the forward reference changes)

if (process.argv.includes('--prod')) {
  process.env.NODE_ENV = 'production'
}

// "prod" no longer makes sense
export const generate = gulp.series(build, serviceWorker);
generate.flags = {
  '--prod': "Set NODE_ENV to 'production'"
};

I understand you provided a pretty basic example and I'd like to see other samples to see how they could be converted to not add another function wrapper around gulp.series/gulp.parallel

gauntface commented 6 years ago

The only other case I hit this is: https://github.com/google/web-starter-kit/blob/wsk-next/gulp-tasks/build.js#L20 I'm using gulp.parallel to manage when streams are done.

The flag I live with as it results in a less verbose gulp config.

phated commented 6 years ago

I'm not sure I understand the level of indirection in that file (viewing from mobile).

The flag I live with to change this - does result in a less verbose set of gulp config.

I'm not sure I understand what's being stated here.

gauntface commented 6 years ago

Sorry - my brain is melting these days.

Basically - using a flag is a good idea.

The indirection of this file - I agree is weird. What I'm doing is that when a use runs gulp build the task will find all of the JS files in gulp-tasks/and if the module exports an object with a build property, it's added to a gulp.parallel() call. This means a file can "add itself" to the gulp build task by simply being in a specific directory.

I guess I'm fighting the use of a stream vs a promise.

gauntface commented 6 years ago

FYI feel free to close this bug as Won't Fix / Working as Intended if this is a use case that shouldn't be supported - I know I'm making abusing the current API's original intention.

trusktr commented 6 years ago
gulp.task('example', () => {
    // Do stuff here
    return gulp.series([....]);  
}()); // <---- RIGHT HERE, the extra ()

Working Gulp 4 example:

const gulp = require('gulp')

function foo(done) {
  console.log('foo')
  done()
}
exports.foo = foo

function bar(done) {
  console.log('bar')
  done()
}
exports.bar = bar

function baz() {

  // ... do stuff ...

  return gulp.series(bar, foo)
}
exports.baz = baz()

where I could've also written the baz task as

gulp.task('baz', () => {

  // ... do stuff ...

  return gulp.series(bar, foo)
}())

Does what you wanted. :)


EDIT: It's important to note that in this case do stuff is evaluated during definition of the task, not when running the task, but that might not matter in most cases.

phated commented 6 years ago

@trusktr your example is unnecessarily complex and doesn't do anything more than you could do outside the task definition. I'm assuming @gauntface wanted to defer the task evaluation until execution time in his example.

phated commented 6 years ago

@gauntface I was just looking over that task you sent us and I totally feel like that functionality best fits as a Custom Registry - I'd be happy to jump on a call if you want to dive deeper on those.

gauntface commented 6 years ago

@phated I'm trying to understand how that helps. Reading through the docs it seems like it's the underpinnings of Gulp, but it's extremely difficult to see how it relates to this issue and / or how it improves the approach linked to earlier on

phated commented 6 years ago

My interpretation of your logic is that you are trying to implement a "require-all" type of logic. Is that incorrect? That's exactly the functionality custom registries were designed for

gauntface commented 6 years ago

What I'm trying to do is have something along the lines of:

gulp --tasks would show the tasks build, watch, dev, prod.

These tasks are largely useless on their own but would look for any files in a specific directory, in this case gulp-tasks/, and would pick up raw functions that are added to appropriate spots in gulp.series tasks or setting up watchers.

For example, gulp-tasks/css.js would expose an object {build: () => {....}, watchGlobs: ['**/*.css']}. The "higher level" tasks build and watch would require these files and make sure they are part of the chain of tasks when gulp build is run and will set up a watcher gulp.watch(task.watchGlobs, task.build).

The idea here is that it decouples the files implementing the chains from the overall build.

Looking at the custom registries, I just ended up complicating the same code as I already have - so I could be using it wrong, but the docs just don't grok for me to see how I'd use them differently.

phated commented 6 years ago

Basically, any time you are warping the way gulp is used (as linked), it's probably much easier to implement a custom registry. e.g. I implemented forward references a couple lines using custom registries. If you look on the org, we have some reference impls.

phated commented 6 years ago

@gauntface I want to make sure there was a clear path forward. Did you get things worked out?

nevercast commented 6 years ago

Just tagging along for the ride here. I've something along the lines of the following:

function deploy() {
  log(`Starting deploy to ${configuration.serverName}`)
  const deployer = createDeployerService()
  return (gulp.series(
                 validateServer(configuration.serverName),
                 deleteFilesFromServer(configuration.serverName),
                 uploadDistToServer(configuration.serverName)
             ))();
}

const deployTask = gulp.series(
  loadConfiguration,
  validateConfiguration,
  buildWebpack,
  jest,
  karma,
  deploy
);

export { deployTask as default };

I've used gulp.series here and invoked it inside deploy to help chain my tasks together. All these tasks return promises so I could just use promise.then.then... but I had gulp.series available, is there a better way to compose this? What is the "Gulp way" of composing nested tasks?

yocontra commented 6 years ago

@nevercast the "gulp way" is to go plain js wherever possible, I would write your stuff as simply:

const deploy = async () => {
  log(`Starting deploy to ${configuration.serverName}`)
  const deployer = createDeployerService()
  await validateServer(configuration.serverName)
  await deleteFilesFromServer(configuration.serverName)
  await uploadDistToServer(configuration.serverName)
}

const deployTask = gulp.series(
  loadConfiguration,
  validateConfiguration,
  buildWebpack,
  jest,
  karma,
  deploy
)

export default deployTask

You only need gulp.series/gulp.parallel when you start needing nesting, stream completion, callbacks, etc. - if you are using simple promises you can just use async/await normal JS stuff.

phated commented 6 years ago

@contra's answer here is really spot on! Sometimes those different APIs won't be promise returning and you can just nest series/parallel calls.

// Notice how this doesn't need to be in a task?
// It could go inside the log task too, I suppose.
const deployer = createDeployerService();

const deployTask = gulp.series(
  loadConfiguration,
  validateConfiguration,
  buildWebpack,
  jest,
  karma,
  gulp.series(
        async () => log(`Starting deploy to ${configuration.serverName}`),
        validateServer(configuration.serverName),
        deleteFilesFromServer(configuration.serverName),
        uploadDistToServer(configuration.serverName)
   )
);

export { deployTask as default };
markstos commented 6 years ago

I'm providing some more related feedback here. I'm working my way through upgrading a gulpfile to Gulp 4 that included about 30 or so tasks, a multi-day project.

In a number of cases, it would have been very convenient to be able to return gulp.series() and gulp.parallel(). While there are a lot of different async patterns out there, it feels like Promises are becoming a dominate pattern with the arrival of async and await in Node.js.

The official API docs for series frankly aren't clear what it returns, they just say it's a composed operation. Well, what's that? Given all the async possibilities, maybe it's a promise under the hood and maybe it's not.

The thing is that in a promise-based flow, promises are returned, and that's an increasingly expected pattern in modern async libraries. This may not been the case several years ago when the Gulp 4 effort was started, but that's the landscape today.

If return gulp.series() was viable, several hours would be saved upgrading Gulp projects like ours to Gulp 4 and I think would fit it will with the growing number of Promise-based Node modules. (Mongoose 5 embraced promises, Express 5 will have promise support).

Here's an example of the gulp tasks I'm trying to update:

So, yeah, it would be really convenient if Promise were returned here.

If that can't happen, the documentation could clarify things:

phated commented 6 years ago

@markstos you're thinking about gulp composition completely wrong - perhaps you should review our brand-new Getting Started guide to help you start thinking in the currently recommended patterns. There's even mention of async/await patterns!

markstos commented 6 years ago

@phated I checked out the link you provided and there is no mention that addresses the basic complexity of needing to compose a gulp.series list at runtime.

Since series and parallel don't accept arrays, it would also be helpful document an example using an the spread operator to show an dynamically generated array can be turned into a function argument list.

It's an unfortunate design choice in 2018 to have standardized all async tasks back into the ancient callback pattern.

The workaround for developers who have more complex needs beyond the "hello world" examples is to bypass gulp.series and gulp.parallel completely.

Multiple await calls natively help serialize async tasks tasks, while the native Promise.all handles running multiple async tasks in parallel.

The value that gulp.series and gulp.parallel offer by unifying multiple async types is negated returning a simple callback function as result. Promises offer greater flexibility, so developers are better off using the various existing libraries that already convert callbacks, streams and events to promises if their needs are all complex.

yocontra commented 6 years ago

@markstos gulp.series and gulp.parallel are simply helpers - you don't need to use them at all where you don't need them. gulp works perfectly fine with async functions and await.

Example gulpfile:

export const someJob = async () => {
  await doSomething()
  await doSomething()
}
export const taskName = async () =>
  Promise.all([
    someJob(),
    someJob()
  ])