Registry sync doesn't handle scoped packages properly
Closed this issue · 10 comments
Hiya,
I recently found this registry syncing module and it appears perfect for my needs. I tried it out on the Xen Orchestra server package, which contains dependencies on scoped packages [1] such as @marsaud/smb2. Unfortunately, for some reason since the scope is part of the package name and not the request, the slash needs to be URL encoded before being requested (without URL encoding things like the @ symbol).
I've created a patch [2] that fixes the sync portion of the tool, but unfortunately the serve portion won't respond to requests for scoped packages that have %2f in their title, because they seem to get converted back to slashes before being handed to the bound routes. I'm not really much of a javascript programmer, so unfortunately I've only been able to patch the sync portion. I'd be happy to run tests on the serve portion if that would help resolve this issue though?
Thanks,
Mike 5:)
[1] https://docs.npmjs.com/misc/scope
[2] https://gist.github.com/ikelos/7f76c81a7fe4e35c6ec63cde1547121b
This link [1] suggests that there's no intention to change this method of encoding scoped package names...
[1] npm/npm#11738
Thanks Mike! I wasn't aware of the scoped package naming at all. You are absolutely correct about Express.js performing urldecoding a bit too "early". I will take a look at this later today and possibly replace the routing part with a custom matcher.
.. and I did it already as it was quite simple after all 😄
Seems to work with my test material. Would you like to try on your setup before I make an updated release ?
Hiya Heiikipora!
Thanks for making that patch, but it has the slight issue of case sensitivity on linux systems. The patch I added hard codes the value to %2F, but when npm requests, it requests with %2f. The options seem to be a) rewrite the serve part to do some case changing, b) rewrite the sync part to store the packages so that they're uniformly accessible or c) change the sync part so that it only ever writes lower case %2f. The third option doesn't feel very robust (if the npm mechanism for urldecoding changes case, the the program will break again), but I'm not sure what the best option to go for is...
Happy to test any solutions you develop (but my rig's at work, so I'll only be able to test on week days),
Mike 5:)
Ah, and I just spotted, the sync feature reads in the package names from the json (which uses slashes) but then doesn't request the package from the upstream registry using %2f (which it needs to in order to resolve the index.json and eventual tarball location). It feels too specific to use replace, but using a full URL encode encodes the @ symbol as well... 5:S Perhaps there should be a handleScopedPackageNames function that can be replace for now and then swapped out later if there's a better way of handling things? The code probably needs checking to ensure it encodes the name properly whenever it's used (as %2f when used in a URL and / everywhere else)...
Ok, so hopefully [1] is a better patch for the sync code. 5:)
[1] https://gist.github.com/ikelos/a38626884d1a7fe057d2ce1f32003c73
Your solution looks good to me. I applied the patch and added a call to encodeScopedPackage() also in formatPrebuilt() (don't have any test packages with both a scoped name and a pre-built binary but it feels right this way ).
Didn't spot the case sensitivity bug as I'm all OS X 😄
Awesome, good spot, thanks! 5:) I haven't tested whether it still works appropriately with the registry-serve program (I swapped to httpd since I already had it running for something else), so it would be worth checking that. Something simple having @marsaud/smb2 in the dependencies seems like the easiest way to test...
Yes, I have verified that :) (tested with the script-suite in folder test/)
I'll push the release to npmjs.com now
Done. The release is v1.2.1.
Thanks @ikelos for your contribution!