Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

Scoped packages #272

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Scoped packages #272

wants to merge 5 commits into from

Conversation

jbeard4
Copy link

@jbeard4 jbeard4 commented Oct 8, 2017

Attempt to resolve issue #271
All tests pass, and includes additional tests for publishing scoped modules.

@jbeard4
Copy link
Author

jbeard4 commented Oct 8, 2017

I see some of the tests I wrote in 02a-publish-scoped.js are failing for travis. I will investigate this.

@iamchrismiller
Copy link

@jbeard4 Any luck tracing those failing tests? I have hit this blocker for scoped packages as well. If you don't have time I can take a look at it.

@jbeard4
Copy link
Author

jbeard4 commented Nov 27, 2017

I haven't investigated why these tests are failing on Travis. They pass on my local. You can pull my branch and try it. If you have time, please feel free to investigate why these tests are failing on travis.

@IgnacioHR
Copy link

Hi,

Thank you for this patch. I was also suffering from #271 and came to the same conclusion as you.

After a quick review on the patch code I think the problem with Travis is that after the patch, you can ONLY store scoped packages and will no longer store no scoped package because of no match of the regex expression.

Unfortunately, I'm not experienced enough on Javascript to propose al alternative patch but at least I can give you short approach in pseudo code that someone can understand and translate to proper javascript

  var f = t.match(/(@[^\/]+\/)?[^\/]+$/)[0]
  var scoped = f != null  // check if f matched and set scoped var to either true or false
  if !scoped then // if not scoped, fallback to previous regular expression
    var f = t.match(/[^\/]+$/)[0]

later, near line 118

var tf = scoped ? 
   [doc.name.replace('/','%2f'), '-', f.replace('/','%2f')] : 
   [doc.name, '-', t.split('/').pop()]

@jbeard4
Copy link
Author

jbeard4 commented Jan 9, 2018

Hi @IgnacioHR, upon further inspection, I found I needed to add an admin user to couch before running tests to publish scoped packages. All tests now pass in Travis, including those for non-scoped packages.
I think this PR could now be merged.

Thank you.

@jbeard4
Copy link
Author

jbeard4 commented Jan 9, 2018

I'm not sure why, but the last travis build ran tests against node versions 0.10, 0.12, iojs, 4 and 5, and failed for verisons 0.10, 0.12, and iojs (but passed for verions 4 and 5). The node versions that failed are very old and no longer supported. I'm not sure why travis ran the tests against these old versions, as the previous build ran tests against node versions 4, 6, and 7, which are listed in the project's .travis.yml configuration file.

@jbeard4
Copy link
Author

jbeard4 commented Jan 9, 2018

Upon further inspection, I see that the .travis.yml file was updated in master in commit 5e12d6a, so that the nodejs versions reverted to an older version. Maybe travis is using .travis.yml file from master, rather than the .travis.yml file from my branch.

@jbeard4 jbeard4 closed this Jan 10, 2018
@jbeard4 jbeard4 reopened this Jan 10, 2018
@jbeard4
Copy link
Author

jbeard4 commented Jan 10, 2018

Fixed travis build for node versions iojs, 0.12, 0.10 by pegging tap to version 10.7.3, which is the last version of tap supported by these older versions of node. This PR should now be OK to merge.

@watilde would it be possible to review this PR? Thank you.

@@ -115,7 +115,7 @@ shows.package = function (doc, req) {
// .../_rewrite/pkg/-/pkg-version.tgz
// or: /pkg/-/pkg-version.tgz
// depending on what requested path is.
var tf = [doc.name, '-', t.split('/').pop()]
var tf = [doc.name.replace('/','%2f'), '-', f.replace('/','%2f')]
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks fine, but can we just use encodeURIComponent instead of regex?

Copy link
Author

@jbeard4 jbeard4 Jan 11, 2018

Choose a reason for hiding this comment

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

This will also uri-encode the "@" as "%40". So, for example, the saved tarball for package name "@scoped/package@0.2.3-alpha" will be available at path "%40scoped%2Fpackage/-/%40scoped%2Fpackage-0.2.3-alpha.tgz", rather than "@Scoped%2Fpackage/-/@Scoped%2Fpackage-0.2.3-alpha.tgz"

I need to check if this maintains compatibility with the npm client.

@watilde
Copy link
Contributor

watilde commented Jan 10, 2018

Looks basically good to me, thanks. // but I'm not a committer

@jbeard4
Copy link
Author

jbeard4 commented Jan 10, 2018

Thanks for the review.

@IgnacioHR
Copy link

After installing this version I could successfully run npm install. But now, I get error 404 when trying to do npm update on the same project where npm install worked. Any idea? The log says

742 http fetch GET 304 http://{serveraddress}.com:5984/registry/_design/app/_rewrite/@mycompany%2fmypackage 103ms (from cache)
743 http fetch GET 404 http://{serveraddress}.com:5984/@mycompany%2fmypackage/-/@mycompany%2fmypackage-1.0.0.tgz 7ms
744 silly fetchPackageMetaData error for @mycompany/mypackage@1.0.0 404 Object Not Found: @mycompany/mypackage@1.0.0

it looks like "_design/app/_rewrite/" is missing in the URL requested by the command.

@jbeard4
Copy link
Author

jbeard4 commented Jan 27, 2018

@IgnacioHR I'll take a look

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants