-
-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #5 by properly encoding the name of scoped packages before requesting their package from npm #6
Changes from all commits
6f08453
69e4607
0dfa460
835df68
ca1065c
234c76d
7798656
1dc06bc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,20 +18,37 @@ function get(url, cb) { | |
}); | ||
} | ||
|
||
function getCleanName(name){ | ||
// Any path components in the name need to be URI encoded, however the @ | ||
// symbol must not be encoded. | ||
// Valid URL: https://registry.npmjs.org/@sindresorhus%2Fdf/ | ||
// Invalid URL: https://registry.npmjs.org/%40sindresorhus%2Fdf | ||
return encodeURIComponent(name).replace(/^%40/, '@'); | ||
} | ||
|
||
function packageIsScoped(name){ | ||
return name[0] === '@'; | ||
} | ||
|
||
module.exports = function (name, version, cb) { | ||
var url = registryUrl(name.split('/')[0]) + name + '/'; | ||
var registry = registryUrl(name.split('/')[0]); | ||
var url = registry + getCleanName(name) + '/'; | ||
|
||
if (typeof version !== 'string') { | ||
cb = version; | ||
version = ''; | ||
} | ||
|
||
if (version && packageIsScoped(name)) { | ||
throw new Error('Fetching a specific version of a scoped package is not allowed by npm.'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't even see this earlier. Thank you for adding a check here. |
||
} | ||
|
||
get(url + version, cb); | ||
}; | ||
|
||
module.exports.field = function (name, field, cb) { | ||
var url = registryUrl(name.split('/')[0]) + | ||
'-/by-field/?key=%22' + name + '%22&field=' + field; | ||
'-/by-field/?key=%22' + getCleanName(name) + '%22&field=' + field; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
|
||
get(url, function (err, res) { | ||
if (err) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,36 +1,55 @@ | ||
'use strict'; | ||
var assert = require('assert'); | ||
var assert = require('chai').assert; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Output when using assert
Output when using chai.assert
The latter is significantly more descriptive and easy to debug. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I live the more descriptive form. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe you have an outdated mocha. I get with an intentional failing assertion:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On (~/.../test/package-json)-(git::issue-5)--> npm test
> package-json@1.2.0 test /home/user/hbetts/workspace/test/package-json
> mocha
public packages
✓ should get the package.json (73ms)
✓ should get the package.json for a specific version
✓ should get the package.json main entry when no version is specified (46ms)
1) get a single field
scoped packages
2) should get the package.json
3) should get the package.json for a specific version
4) should get the package.json main entry when no version is specified
5) get a single field
3 passing (266ms)
5 failing Checking outdated packages: (~/.../test/package-json)-(git::issue-5)--> npm outdated
Package Current Wanted Latest Location
got 3.3.1 3.3.1 4.1.1 got There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sindresorhus is the outdated mocha suggestion to me in regards to lacking useful output on failed assertions? I am using whichever version is coming with npm install for the package. In my case, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should I disable the specific version test for the scoped package, and log a message to the command line if a user tries to do so for now so that we can get the rest of this patch deployed? We could then enable specific version functionality for scoped packages in the future when the downstream issue with npm is resolved. @destroyerofbuilds thanks for digging into that issue, and good call checking out the response headers There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I received a little feedback in IRC, but not resolution: http://logs.nodejs.org/npm/latest#19:04:58.872 I'm for disabling the offending test, and filing an issue to follow back around with the issue later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @destroyerofbuilds I just pushed a commit to throw an error if a version is requested for a scoped package, and update the tests to check for the error in the scoped package case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
👍 @lzilioli Can you also update the readme to mention this limitation? |
||
var packageJson = require('./'); | ||
|
||
it('should get the package.json', function (cb) { | ||
packageJson('pageres', function (err, json) { | ||
assert(!err, err); | ||
assert.strictEqual(json.name, 'pageres'); | ||
cb(); | ||
}); | ||
}); | ||
function runTests(cfg){ | ||
|
||
it('should get the package.json for a specific version', function (cb) { | ||
packageJson('pageres', '0.1.0', function (err, json) { | ||
assert(!err, err); | ||
assert.strictEqual(json.version, '0.1.0'); | ||
cb(); | ||
it('should get the package.json', function (cb) { | ||
packageJson(cfg.name, function (err, json) { | ||
assert(!err, err); | ||
assert.strictEqual(json.name, cfg.name); | ||
cb(); | ||
}); | ||
}); | ||
}); | ||
|
||
it('should get the package.json main entry when no version is specified', function (cb) { | ||
packageJson('pageres', function (err, json) { | ||
assert(!err, err); | ||
assert(json._id); | ||
cb(); | ||
it('should get the package.json for a specific version', function (cb) { | ||
if(cfg.version === 'expect-throw') { | ||
assert.throws(packageJson.bind(null, cfg.name, cfg.version)); | ||
cb(); | ||
} else { | ||
packageJson(cfg.name, cfg.version, function (err, json) { | ||
assert(!err, err); | ||
assert.strictEqual(json.version, cfg.version); | ||
cb(); | ||
}); | ||
} | ||
}); | ||
}); | ||
|
||
it('get a single field', function (cb) { | ||
packageJson.field('pageres', 'description', function (err, res) { | ||
assert(!err, err); | ||
assert(typeof res === 'string'); | ||
assert(/screenshots/.test(res)); | ||
cb(); | ||
it('should get the package.json main entry when no version is specified', function (cb) { | ||
packageJson(cfg.name, function (err, json) { | ||
assert(!err, err); | ||
assert(json._id); | ||
cb(); | ||
}); | ||
}); | ||
|
||
} | ||
|
||
// test spec | ||
var spec = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Glad to see all test cases applied to both scoped, and un-scoped, packages. |
||
'public packages': { | ||
name: 'pageres', | ||
version: '0.1.0', | ||
descriptionRe: /screenshots/ | ||
}, | ||
'scoped packages': { | ||
name: '@sindresorhus/df', | ||
version: 'expect-throw', | ||
descriptionRe: /Get free disk space info from/ | ||
} | ||
}; | ||
|
||
// test runner | ||
Object.keys(spec).forEach(function(key){ | ||
describe(key, runTests.bind(null, spec[key])); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really sure what this function is supposed to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. It's purpose is not clear, and tbh, I do not recall either. It is definitely required, though. I will re-discover why it was needed and add comments as appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lzilioli thank you for the updated documentation for this method.