Skip to content

Commit

Permalink
HAHA wow. it installed
Browse files Browse the repository at this point in the history
  • Loading branch information
jonathanong committed Dec 16, 2013
1 parent 6fe72eb commit 0d62622
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 1 deletion.
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
build
components
node_modules
3 changes: 2 additions & 1 deletion lib/package.js
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ Package.prototype.getJSON = function* (){
else err.message += ' in ' + url;
throw err;
}
if (res.statusCode !== 400) throw error(res.res, url);
if (res.statusCode !== 200) throw error(res.res, url);
return res.body;
};

Expand Down Expand Up @@ -336,6 +336,7 @@ Package.prototype.reallyInstall = function* (){
yield* this.writeFile('component.json', json);

This comment has been minimized.

Copy link
@yields

yields Dec 16, 2013

why the *

This comment has been minimized.

Copy link
@jonathanong

jonathanong Dec 16, 2013

Author Contributor

it's called delegation. let me find a link

This comment has been minimized.

Copy link
@jonathanong

jonathanong Dec 16, 2013

Author Contributor

http://stackoverflow.com/questions/17491779/delegated-yield-in-generator-functions

basically, it avoids extra co calls. slightly faster (since you don't nest co.call(generator) calls) and i like it because it's basically type checking, but TJ doesn't like it.

This comment has been minimized.

Copy link
@TooTallNate

TooTallNate Dec 16, 2013

Contributor

yield delegation when using co has been entirely unneeded so far in my experience. What is writeFile() returning?

This comment has been minimized.

Copy link
@jonathanong

jonathanong Dec 16, 2013

Author Contributor

nothing, hence no var x =. i just copied what it was before into generators.

This comment has been minimized.

Copy link
@jonathanong

jonathanong Dec 16, 2013

Author Contributor

@TooTallNate yeah, for me it's just a style preference. not difficult to remove if you guys don't want it (though i think it's easier to read and understand when there are red *s everywhere)

This comment has been minimized.

Copy link
@TooTallNate

TooTallNate Dec 16, 2013

Contributor

I'm kinda -1 with TJ on that one. It's not obvious to me when you're using yield vs. yield * (though I understand it's for optimization purposes).

This comment has been minimized.

Copy link
@jonathanong

jonathanong Dec 16, 2013

Author Contributor

what's not obvious? that there's a * there at all? sublime text highlights it so it's obvious to me.

This comment has been minimized.

Copy link
@TooTallNate

TooTallNate Dec 16, 2013

Contributor

It's more the why of one vs. the other. Might be confusing for outside contributors or something.

Now that I've sat on it for a minute and understand it, it makes more sense to me so I'm ok with it I think...

This comment has been minimized.

Copy link
@jonathanong

jonathanong Dec 16, 2013

Author Contributor

in my experience, i like to know what i'm yielding. if you yield thunk, you might run into context issues, so i know to only either do yield thunk.bind(context) or yield* genfun(). however, that's only a problem when you delegate the co engine from somewhere else like koa.

i don't mind removing it. not like it matters performance wise for this. if you guys want me to remove it, LMK.

yield* this.getFiles(files);
yield* this.channel.flush();
return
}

// failed
Expand Down
10 changes: 10 additions & 0 deletions test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,14 @@ describe('Package.js', function () {
pkg.url('index.js').should.eql('https://raw.github.com/' + repo + '/' + version + '/index.js');
});
});

describe('Install', function () {
var pkg = new Package('component/domify', '1.1.1', {
dest: path.join(__dirname, '..', 'components')
})

it('should install', function (done) {
pkg.end(done)
})
})
});

0 comments on commit 0d62622

Please sign in to comment.