Aliases redux

[phated]

@erikkemperman will you take a look at this. I've updated your branch with my thoughts on using a taskWrapper function. Your idea to use meta.orig in the createExtensions module to capture the lastRun time is what brought this back to my mind. It was the original solution that allowed us to do task aliases but I removed it to make the lastRun implementation cleaner, but with your usage of meta.orig, we can keep it simple and support the taskWrapper function.

We still run into the issue that aliased tasks will still have the lastRun time of each other (since it's the same function), but maybe that is the correct, and expected behavior.

[phated]

Added a test for the lastRun + alias case.

[erikkemperman]

@phated Thanks for taking the time!

I really like not having the second WeakMap, that always felt kludgy to me.

Is there any benefit to having an actual function taskWrapper rather than a simple zero-arg bind like I had proposed? I am assuming that in terms of GC, this function acts the same as with a bind, which is to say the same as the original fn (needed for the weak map to retain it properly)?

Also, what is the rationale behind passing this as the context arg to fn.apply? The way I read it it'll always be undefined, or did you intend to pass in the undertaker/gulp instance for some reason?

The function is wrapped unconditionally, but I thought it was not strictly necessary in case the current registry set returns a different function than the one we pass in (i.e. wrapping it for us already, and thus relieving us of the need for a wrapper to ensure a unique key in the weak map).

The code is slightly simpler to follow this way, of course, but it involves one extra function call which is not always needed. I'm not sure if it would be a common thing to do, but I for one expect to always have a custom registry (re-usable tasks) and probably it will wrap the function (as in the examples, to allow configuration).

Come to think of it, misbehaved registries might return the same function for different names from their set method. That would break the current master as well as both of these PRs. We could check if the return value has no key in the weak map, but eew.

By the way, this implementation has a different outcome for the edge case I mentioned here, because unlike my implementation you're passing the wrapper into _registry.set instead of the original function. Like I said though, I'm not sure what users expect to happen there, or if it is even remotely likely that this will ever be an issue for anyone.

I also couldn't say what behaviour people would expect from last-run, haven't played around much with that yet. But since we're effectively running the exact same task, I think I might agree that having aliases share their last run time is reasonable enough, so as long as it is documented clearly. Unless perhaps you imagine users might be creating aliases specifically to get around last-run, or something like that?

For what it's worth, if this does turn out to be problematic, I think it would be rather easy to fix in createExtensions by doing something like this in the Storage constructor

this.fn = meta.orig || fn;
this.lastRunKey = fn;

and then using this.lastRunKey for our argument to captureLastRun and releaseLastRun.

[erikkemperman]

Never mind the question about this in fn.apply, I played around with it a bit more and I get it now.

[phated]

:P I will have a follow up post shortly

[phated]

@erikkemperman I will answer from top to bottom on your post.

The benefit to using taskWrapper instead of .bind is that bind is still extremely slow in V8. Other engines have made improvements but it seems the V8 team doesn't care. The function wrapper is the same outcome but much faster.

Passing this in the .apply allows a custom registry to bind the wrapper and everything still works correctly. I did this just out of habit (when monkeypatching, I always use the line return fn.apply(this, arguments);) but it has use too :stuck_out_tongue:

The wrapping done unconditionally (and passing the taskWrapper to the registry) forces a decoupling of registry from task function. With the taskWrapper, a custom registry can't add properties that an end user could then access if they retrieved the task function with gulp.task('taskName').

Custom registries are seen as an advanced feature and I don't expect most people to use them. However, I think a lot of people use task aliases (not sure why). I also think the extra function wrapper will be a negligible performance hit; It was in undertaker for a long time before I removed it and no one mentioned a perf difference between the two.

I think both of our implementations handle the edge case you mentioned. Current gulp prints "real" when gulp real is called and "alias" when gulp alias. As you mentioned in your PR, that is the same behavior as your impl and I just tested with this impl with the same results too. I think this is expected behavior when using that custom registry. Again, they are an advanced feature and that was just a PoC.

I think lastRun should be fine for the most part and I agree that if people start to use it and think it is a bug, it is easy to patch using some unique identifier. The current implementation is due to wanting to support string names and functions passed in.

[phated]

@erikkemperman if my above response covers all of your questions/concerns, I'd like to get this into a new version today

[erikkemperman]

@phated Definitely, and then some. Didn't know bind was especially slow on V8, but then I suppose I might never have used it in critical path. So yup, looks good to me, thanks again!

[phated]

@erikkemperman Thanks for reviewing and the great questions. Got me really thinking about everything.

[erikkemperman]

@phated Happy to play along, although I should probably think slightly harder about some questions before posting them pre-coffee :-D