Skip to content

Commit

Permalink
Breaking: Return wrapped task instead of original function, add unwra…
Browse files Browse the repository at this point in the history
…p method to wrapper, update tests
  • Loading branch information
phated committed Jun 22, 2016
1 parent c0f8fff commit 67fd02c
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 23 deletions.
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ to create the registry for this instance.
Both a `getter` and `setter` for tasks.

If a string (`taskName`) is given as the only argument, it behaves as a `getter`
and returns the registered function.
and returns the wrapped task (not the original function). The wrapped task has a `unwrap`
method that will return the original function.

If a function (`fn`) and optionally a string (`taskName`) is given, it behaves as
a `setter` and will register the task by the `taskName`. If `taskName` is not
Expand Down
16 changes: 1 addition & 15 deletions lib/get-task.js
Original file line number Diff line number Diff line change
@@ -1,21 +1,7 @@
'use strict';

var metadata = require('./helpers/metadata');

function get(name) {
var wrapper = this._registry.get(name);

if (!wrapper) {
return;
}

var meta = metadata.get(wrapper);

if (meta) {
return meta.orig;
}

return wrapper;
return this._registry.get(name);
}

module.exports = get;
8 changes: 8 additions & 0 deletions lib/last-run.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

var retrieveLastRun = require('last-run');

var metadata = require('./helpers/metadata');

function lastRun(task, timeResolution) {
if (timeResolution == null) {
timeResolution = process.env.UNDERTAKER_TIME_RESOLUTION;
Expand All @@ -12,6 +14,12 @@ function lastRun(task, timeResolution) {
fn = this._getTask(task);
}

var meta = metadata.get(fn);

if (meta) {
fn = meta.orig || fn;
}

return retrieveLastRun(fn, timeResolution);
}

Expand Down
5 changes: 5 additions & 0 deletions lib/set-task.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ function set(name, fn) {
return fn.apply(this, arguments);
}

function unwrap() {
return fn;
}

taskWrapper.unwrap = unwrap;
taskWrapper.displayName = name;

var meta = metadata.get(fn) || {};
Expand Down
29 changes: 22 additions & 7 deletions test/task.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,20 +25,20 @@ describe('task', function() {

it('should register a named function', function(done) {
taker.task(noop);
expect(taker.task('noop')).to.equal(noop);
expect(taker.task('noop').unwrap()).to.equal(noop);
done();
});

it('should register an anonymous function by string name', function(done) {
taker.task('test1', anon);
expect(taker.task('test1')).to.equal(anon);
expect(taker.task('test1').unwrap()).to.equal(anon);
done();
});

it('should register an anonymous function by displayName property', function(done) {
anon.displayName = '<display name>';
taker.task(anon);
expect(taker.task('<display name>')).to.equal(anon);
expect(taker.task('<display name>').unwrap()).to.equal(anon);
delete anon.displayName;
done();
});
Expand All @@ -54,7 +54,7 @@ describe('task', function() {

it('should register a named function by string name', function(done) {
taker.task('test1', noop);
expect(taker.task('test1')).to.equal(noop);
expect(taker.task('test1').unwrap()).to.equal(noop);
done();
});

Expand All @@ -65,7 +65,22 @@ describe('task', function() {

it('should get a task that was registered', function(done) {
taker.task('test1', noop);
expect(taker.task('test1')).to.equal(noop);
expect(taker.task('test1').unwrap()).to.equal(noop);
done();
});

it('should get the wrapped task, not original function', function(done) {
var registry = taker.registry();
taker.task('test1', noop);
expect(taker.task('test1').unwrap).to.be.a.function();
expect(taker.task('test1')).to.equal(registry.get('test1'));
done();
});

it('provides an `unwrap` method to get the original function', function(done) {
taker.task('test1', noop);
expect(taker.task('test1').unwrap).to.be.a.function();
expect(taker.task('test1').unwrap()).to.equal(noop);
done();
});

Expand All @@ -79,15 +94,15 @@ describe('task', function() {
function fn() {}
fn.displayName = 'test1';
taker.task(fn);
expect(taker.task('test1')).to.equal(fn);
expect(taker.task('test1').unwrap()).to.equal(fn);
done();
});

it('should allow different tasks to refer to the same function', function(done) {
function fn() {}
taker.task('foo', fn);
taker.task('bar', fn);
expect(taker.task('foo' )).to.equal(taker.task('bar'));
expect(taker.task('foo').unwrap()).to.equal(taker.task('bar').unwrap());
done();
});

Expand Down

4 comments on commit 67fd02c

@Xiphe
Copy link

@Xiphe Xiphe commented on 67fd02c Jun 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which problems does this fix? Am I, as a consumer, expected to unwrap the tasks? Or what kind of implementation breaks when updating to 1.0.0?

My custom registry seems to work fine (ref Xiphe/gulp-toolbox-registry#64), but I'm not really sure if I miss some implications here :)

@phated
Copy link
Member Author

@phated phated commented on 67fd02c Jun 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Xiphe the only thing that this changes (for gulp) is that gulp.task('css') !== cssFn where it used to. Also, aliased tasks get wrapped an extra time. unwrap() isn't really expected to be used (other than testing) but I wanted to allow people to get the original function if they needed to for some reason.

Side note: custom registries have always received the wrapped task and didn't operate on the original function.

@phated
Copy link
Member Author

@phated phated commented on 67fd02c Jun 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The biggest implication of this is that it solves gulpjs/gulp#1492

@Xiphe
Copy link

@Xiphe Xiphe commented on 67fd02c Jun 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 great! thanks for the explanation

Please sign in to comment.