Skip to content
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

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

Closed
gauntface opened this issue Feb 4, 2018 · 20 comments
Closed

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

gauntface opened this issue Feb 4, 2018 · 20 comments

Comments

@gauntface
Copy link

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
Copy link
Member

phated commented Feb 4, 2018

@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
Copy link
Author

gauntface commented Feb 6, 2018

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
Copy link
Member

phated commented Feb 6, 2018

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
Copy link
Author

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
Copy link
Author

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
Copy link

trusktr commented Feb 8, 2018

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
Copy link
Member

phated commented Feb 11, 2018

@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
Copy link
Member

phated commented Feb 14, 2018

@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
Copy link
Author

@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
Copy link
Member

phated commented Feb 23, 2018

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
Copy link
Author

gauntface commented Feb 23, 2018

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
Copy link
Member

phated commented Feb 23, 2018

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
Copy link
Member

phated commented May 13, 2018

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

@nevercast
Copy link

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
Copy link
Member

@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
Copy link
Member

phated commented May 16, 2018

@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
Copy link

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:

  • We have a "build-less" task that's called by multiple other tasks. Depending on who calls it and an environment variable, it might build a single Less file or a Less file for every customer. If we need to build a Less file for every customer, we dynamically generate a gulp task for every customer (so we can see the per-customer progress in the output), and they run them in parallel using gulp.parallel().

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

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

  • The Series documentation could clarify that it's not suitable for returning from a function.
  • The Tasks docs could document the various types of async functions that are supported. (The list of options isn't even linked to from this page). It could also be useful to mention there that functions that return gulp.parallel() or gulp.series() aren't valid tasks.

@phated
Copy link
Member

phated commented Oct 24, 2018

@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
Copy link

markstos commented Oct 25, 2018

@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
Copy link
Member

yocontra commented Oct 25, 2018

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

@gulpjs gulpjs locked as resolved and limited conversation to collaborators Oct 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants