Fix #5 by properly encoding the name of scoped packages before requesting their package from npm #6

lzilioli · 2015-07-27T01:42:00Z

This PR contains 1 breaking test case for the scoped package.

…y, and encoding the package name

sindresorhus · 2015-07-27T01:44:00Z

index.js

@@ -18,8 +18,20 @@ function get(url, cb) {
 	});
 }

+function getCleanName(name){
+	name = name.split('@');


sindresorhus · 2015-07-27T01:47:32Z

Thanks for the PR :)

sindresorhus · 2015-07-27T01:47:41Z

Can you use a descriptive pull request title?

lzilioli · 2015-07-27T19:00:08Z

@sindresorhus I pushed an additional commit addressing you feedback, and changed the PR title to be more descriptive. Keep in mind that there is still a failing test case on this branch.

ghost · 2015-07-28T23:51:36Z

test.js

+}
+
+// test spec
+var spec = {


Glad to see all test cases applied to both scoped, and un-scoped, packages.

ghost · 2015-07-29T00:00:10Z

I like how every sub-test fails with a different error. I guess at least it's the same test case.

…heck for this

sindresorhus · 2015-08-03T21:18:27Z

@lzilioli Can you fix the tests?

ghost · 2015-08-14T01:26:01Z

@sindresorhus and @lzilioli , I filed #7 as a follow-up to the scoped package metadata end-point.

sindresorhus · 2015-08-21T07:59:01Z

@lzilioli friendly ping :)

lzilioli · 2015-08-21T15:32:34Z

Apologies! I have been on vacation the past two weeks, and did not have access to my computer most of that time. I am taking a look at the test cases now. I was surprised to learn that these test cases were failing, as I did not believe my changes would be responsible for breaking those cases. I performed a git bisect, and the following commit was identified as faulty:

[package-json ((de39fa8...)|BISECTING) Fri Aug 21 10:27:57]
$ git bisect bad head
de39fa84f971ed3f2dd9bd796657fbd8228c47f0 is the first bad commit
commit de39fa84f971ed3f2dd9bd796657fbd8228c47f0
Author: Sindre Sorhus <sindresorhus@gmail.com>
Date:   Thu Jun 4 15:42:46 2015 +0200

    add support for getting a single field from package.json

This seems to indicate that npm may have changed something on their end with respect to the single-field endpoint. Is there documentation for this on their site? If so, I can update the single-field url we are fetching.

Am I missing something here?

lzilioli · 2015-09-11T18:18:43Z

@sindresorhus friendly ping

ghost · 2015-09-11T18:22:53Z

@lzilioli please see my comment here. The by-field API end-point has been removed. Therefore these unit tests for that feature, which make actual calls to the registry, will fail.

My vote would be to add an extra commit to your merge request to comment out those tests. That would allow us to move more quickly with getting this fix out. However, given that the by-field end-point is gone, the entire commit adding that functionality should be reverted and a new release cut.

hutson · 2015-09-22T02:16:48Z

index.js


 	if (typeof version !== 'string') {
 		cb = version;
 		version = '';
 	}

+	if(version && packageIsScoped(name)) {


@lzilioli for style consistency, can you add a space after if?

hutson · 2015-09-22T02:17:46Z

index.js


 	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.');


I didn't even see this earlier. Thank you for adding a check here.

hutson · 2015-09-22T02:22:29Z

I've filed #8 to revert the by-field feature.

lzilioli · 2015-09-22T02:31:38Z

@hbetts @destroyerofbuilds I just pushed two commits. One to address the space for consistency, and the other to remove the tests that it looks like will be reverted in #8

lzilioli · 2015-10-06T19:50:39Z

@sindresorhus Any plans to release this?

Luke Zilioli and others added 3 commits June 26, 2015 20:54

Fix sindresorhus#5 by ensuring proper number of slashes after registr…

6f08453

…y, and encoding the package name

Add tests for public scoped package

69e4607

Fix test and integrate feedback from @destroyerofbuilds

0dfa460

lzilioli mentioned this pull request Jul 27, 2015

Doesn't work properly for scoped packages #5

Closed

sindresorhus reviewed Jul 27, 2015
View reviewed changes

index.js

@@ -18,8 +18,20 @@ function get(url, cb) {

});

}

function getCleanName(name){

name = name.split('@');

Copy link

Owner

sindresorhus Jul 27, 2015

Choose a reason for hiding this comment

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

Why?

address code review feedback

835df68

lzilioli changed the title ~~Fix #5~~ Fix #5 by properly encoding the name of scoped packages before requesting their package from npm Jul 27, 2015

ghost reviewed Jul 28, 2015
View reviewed changes

test.js

}

// test spec

var spec = {

Copy link

ghost Jul 28, 2015

Choose a reason for hiding this comment

The 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.

Luke Zilioli added 2 commits July 29, 2015 12:23

simplify getCleanName

ca1065c

Throw error if version requested for scoped package, update test to c…

234c76d

…heck for this

ghost mentioned this pull request Aug 5, 2015

Can't access scoped metadata from registry npm/npm#9164

Closed

ghost mentioned this pull request Aug 14, 2015

Package Metadata URL May be Deprecated #7

Closed

hutson reviewed Sep 22, 2015
View reviewed changes

Luke Zilioli added 2 commits September 21, 2015 21:29

space for consistency

7798656

remove soon to be reverted tests

1dc06bc

sindresorhus closed this in 3e7500e Oct 9, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #5 by properly encoding the name of scoped packages before requesting their package from npm #6

Fix #5 by properly encoding the name of scoped packages before requesting their package from npm #6

lzilioli commented Jul 27, 2015

sindresorhus Jul 27, 2015

sindresorhus commented Jul 27, 2015

sindresorhus commented Jul 27, 2015

lzilioli commented Jul 27, 2015

ghost Jul 28, 2015

ghost commented Jul 29, 2015

sindresorhus commented Aug 3, 2015

ghost commented Aug 14, 2015

sindresorhus commented Aug 21, 2015

lzilioli commented Aug 21, 2015

lzilioli commented Sep 11, 2015

ghost commented Sep 11, 2015

hutson Sep 22, 2015

hutson Sep 22, 2015

hutson commented Sep 22, 2015

lzilioli commented Sep 22, 2015

lzilioli commented Oct 6, 2015

Fix #5 by properly encoding the name of scoped packages before requesting their package from npm #6

Fix #5 by properly encoding the name of scoped packages before requesting their package from npm #6

Conversation

lzilioli commented Jul 27, 2015

sindresorhus Jul 27, 2015

Choose a reason for hiding this comment

sindresorhus commented Jul 27, 2015

sindresorhus commented Jul 27, 2015

lzilioli commented Jul 27, 2015

ghost Jul 28, 2015

Choose a reason for hiding this comment

ghost commented Jul 29, 2015

sindresorhus commented Aug 3, 2015

ghost commented Aug 14, 2015

sindresorhus commented Aug 21, 2015

lzilioli commented Aug 21, 2015

lzilioli commented Sep 11, 2015

ghost commented Sep 11, 2015

hutson Sep 22, 2015

Choose a reason for hiding this comment

hutson Sep 22, 2015

Choose a reason for hiding this comment

hutson commented Sep 22, 2015

lzilioli commented Sep 22, 2015

lzilioli commented Oct 6, 2015