Fix: Do not set displayName.

[coreyfarrell]

This will allow gulp-cli to preferentially use displayName to calculate the name of the task. Previously when displayName was set in series and parallel this caused tasks declared with exports to have the wrong name. I've removed setting of displayName from set-task.js for consistency, this is not required for the fix.

[coreyfarrell]

Unfortunate the way coveralls considers coverage to have dropped by percentage only. This patch deletes one line that was previously covered, so 204/205 lines covered is less than the previous 205/206 lines covered.

[phated]

Wow, I need to make that threshold more forgiving 😂

[phated]

@coreyfarrell I have this little voice in the back of my head that says we added the task wrapper for a specific reason related to the gulp-cli. I can't quite remember what that was and have been trying to do some code spelunking to find it. Maybe @sttk or @erikkemperman remembers why we did it.

[phated]

@coreyfarrell so I've been trying to do some code-spelunking to figure this out. Currently all tests pass (here and in gulp-cli) when I pull in your changes. I think we might not be testing all the edge cases. @sttk do you know what these changes would effect?

[erikkemperman]

Sorry, I meant to reply here but didn’t get around to it... If I remember correctly the displayName is only populated in undertaker so it has a default, to be overridden if the user wants. But if I understand the issue properly, the former actually breaks the latter in the new style of exporting tasks.

I think the proposed change is probably all right; the only edge case I can think of is the exported tree as used by external tooling (e.g. JetBrains had something I believe) might get out of whack and not reflect the latest displayName if it’s set post-export?

But then I’m not sure if those tools are dealing with the exports correctly at the moment, I haven’t used them myself in a while.

Would it be possible to make the function passed to the task “constructor” use a setter for displayName so that we can update the tree?

Apologies if this doesn’t make sense, it’s been a while since I’ve thought about this stuff.

[sttk]

I have not checked this yet. Please give me some time to check.

[sttk]

I've checked this.

For gulp-cli, this PR effects nothing because gulp-cli receives task names from Undertaker#tree (for --tasks, --tasks-simple, --tasks-json) or events (for start/finish/error logs). gulp-cli doesn't obtain a task name from a task function or a wrapper function directly now. (To fix gulp's issue #2270, only lib/shared/register-exports.js will do so, as @coreyfarrell said.)

Only one thing which concerns me is about the result of gulp.task(taskname).displayName. This always returns a correct task name now, but becomes to return undefined by this PR. I think that this modifications of parallel.js and series.js is ok but of set-task.js is not.

import gulp from 'gulp';
function testLocal() { return Promise.resolve(); }
console.log(gulp.task("testLocal").name);  // => "taskWrapper" (current & this PR)
console.log(gulp.task("testLocal").displayName); // => "testLocal" (current) ==> undefined (this PR)
console.log(gulp.task("testLocal").unwrap().name); // => "testLocal" (current & this PR)
console.log(gulp.task("testLocal").unwrap().displayName); => // undefined (current & this PR)

import gulp from 'gulp';
function testLocal() { return Promise.resolve(); }
testLocal.displayName = 'test-local';
gulp.task(testLocal)
console.log(gulp.task("test-local").name);  // => "taskWrapper" (current & this PR)
console.log(gulp.task("test-local").displayName); // => "test-local" (current) ==> undefined (this PR)
console.log(gulp.task("test-local").unwrap().name);  // => "testLocal" (current & this PR)
console.log(gulp.task("test-local").unwrap().displayName); // => "test-local" (current & this PR)

import gulp from 'gulp';
function testLocal() { return Promise.resolve(); }
testLocal.displayName = 'test-local';
gulp.task("test", testLocal)
console.log(gulp.task("test").name);  // => "taskWrapper" (current & this PR)
console.log(gulp.task("test").displayName); // => "test" (current) ==> undefined (this PR)
console.log(gulp.task("test").unwrap().name);  // => "testLocal" (current & this PR)
console.log(gulp.task("test").unwrap().displayName);  // => "test-local" (current & this PR)

[coreyfarrell]

I've removed the change from lib/set-task.js.

[phated]

I think I might remember why we were creating taskWrapper: the WeakMap shim attaches properties to a function if the platform doesn't support WeakMaps by default and I didn't want to modify the original function (which also breaks if it is frozen or inextensible). I'm going to look into this some more.

(edit): the above shouldn't affect series and parallel because they generate their own function wrappers, I think. Still need to do some digging.

[phated]

@coreyfarrell I remembered why I had added the displayName to functions generated by these 2 methods: Because bach returns named functions from the methods (see https://github.com/gulpjs/bach/blob/master/lib/series.js#L23-L25) and I wanted to obscure those.

I happened to remember this because I wanted to write a test like:

taker.task(taker.series(fn1, fn2));

I was expecting this to throw because we removed the displayName but they can still be registered because those function names are being set. I think I'm going to remove the named functions inside bach (at a later date) and make it so you can't directly register the results of a series/parallel call. I think ensuring they are always named by users is a better default.

[phated]

@coreyfarrell I've merged this but it's late so I'll publish it tomorrow or Sunday.

[phated]

@coreyfarrell published as 1.2.1 - feel free to work on the needed PR inside gulp-cli

[phated]

And thanks for digging into this!

[coreyfarrell]

Posted to gulpjs/gulp-cli#189.