V2 #14
V2 #14
Conversation
BREAKING CHANGE: Promises are now Promises, rather than blue birds. Fix #13
Just use a no-op 'logger' if there isn't one in the options.
Note: floating a dependency on a git fork until npm/bin-links#14 lands.
|
Note: do not land yet. New goal for v2 is to refactor this module so that it isn't smeared out across so many packages. (Most especially: gentle-fs, which is a junk-drawer of sorts.) |
|
Ok, this is ready for review now. Pulled in everything that was smeared out across gentle-fs and here, organized the codebase a bit more, and ironed out all the edge cases. |
README.md
Outdated
| @@ -38,10 +38,12 @@ jump in if you'd like to, or even ask us questions if something isn't clear. | |||
|
|
|||
| ### API | |||
|
|
|||
| #### <a name="binLinks"></a> `> binLinks(pkg, folder, global, opts, cb)` | |||
| #### <a name="binLinks"></a> `> binLinks(pkg, folder, global, opts)` | |||
There was a problem hiding this comment.
The function you created destructures a single parameter.
| #### <a name="binLinks"></a> `> binLinks(pkg, folder, global, opts)` | |
| #### <a name="binLinks"></a> `> binLinks({ pkg, folder, global, opts })` |
README.md
Outdated
| binLinks(pkg, folder, global, opts).then(() => console.log('bins linked!')) |
There was a problem hiding this comment.
Should be denoting an object parameter, than positional params
| binLinks(pkg, folder, global, opts).then(() => console.log('bins linked!')) | |
| binLinks({ pkg, folder, global, opts }).then(() => console.log('bins linked!')) |
| "bugs": { | ||
| "url": "https://github.com/npm/bin-links/issues" | ||
| }, | ||
| "homepage": "https://github.com/npm/bin-links#readme", |
There was a problem hiding this comment.
nit: We should probably keep this "homepage" property as npmjs.com uses this in the UI.
There was a problem hiding this comment.
It gets the default from the repository field, so there's no need for it.
Qv: https://github.com/npm/arborist/blob/master/package.json vs http://npm.im/@npmcli/arborist
| return SKIP // skip it, already set up like we want it. | ||
|
|
||
| target = resolve(dirname(to), target) | ||
| if (target.indexOf(path) === 0 || force) |
There was a problem hiding this comment.
Is it possible for target.indexOf(path) === 0 to be false as well as force? If that were the case, this closer never returns anything, which means we're assuming that's acceptable for line 54. The "else" case that is caused would always want symlink(from, to, 'file').then(() => true) to happen.
This seems correct, but just thought I'd say it out loud and confirmation.
There was a problem hiding this comment.
If there's an stTo, and we're not in force mode, and we're not allowed to delete it, then we just don't delete it. Then the promise resolves, and the symlink() fails with EEXIST, which is the msot appropriate possible error message.
So yes, working as intended.
|
Updated docs. Merging and publishing. |
Native promises and updated CLI environment.