Skip to content

Conversation

KidkArolis
Copy link
Contributor

First attempt at fixing #1895. Discussed in more detail in #1666.

The issue this tries to fix is that rewriting npmjs.org > yarnpkg.com will confuse any logic that relies on reading per registry configuration from the .npmrc file, such as reading authTokens.

I thinks this PR might affect how the lock files get generated, not sure if that's a problem.

@bestander
Copy link
Member

@KidkArolis, this change looks reasonable but it is hard to see the side effects.
What Test Scenario would you run for this PR?
Any unit test?

@KidkArolis KidkArolis force-pushed the fix-private-npm branch 2 times, most recently from 89865bd to 32c4d59 Compare November 19, 2016 16:04
@KidkArolis
Copy link
Contributor Author

Hm, so I tried adding a test, but then realised I couldn't reproduce the issue discussed in #1666. I bought an npm subscription to investigate this further.

I think what happens is this:

  1. Resolve registry for @kidkarolis/yarn-npm-issue to https://registry.npmjs.org/.
  2. Construct a request URL, https://registry.npmjs.org/@kidkarolis%2fyarn-npm-issue.
  3. Package is scoped and request URL matches registry URL, therefore we look for a token.
  4. Find token in ~/.npmrc for //registry.npmjs.org/.
  5. Send the token.
  6. Now we need to download the tarball via the rewritten url https://registry.yarnpkg.com/@kidkarolis/yarn-npm-issue/-/yarn-npm-issue-1.0.1.tgz.
  7. Resolve registry for this url to https://registry.yarnpkg.com this time.
  8. Package is scoped and request URL matches registry URL (this time yarnpkg.com), therefore we look for a token.
  9. Don't find a token for https://registry.yarnpkg.com.
  10. Look for DEFAULT_REGISTRY (registry.npmjs.org) and find it.
  11. Send the npm registry token to yarn's server.

So, yeah, it works though, so in a way, this isn't an issue.

Let me see if I can summon @SEAPUNK and ask him to reproduce.

@KidkArolis
Copy link
Contributor Author

@SEAPUNK , if you follow these instructions, does it work for you?

  1. Download and build yarn's master.
  2. mv ~/.npmrc ~/.npmrc.bak
  3. npm login
  4. yarn add @xbpf/suppliers

@SEAPUNK
Copy link

SEAPUNK commented Nov 29, 2016

@KidkArolis OH, didn't notice this until now. I'll try it out later tonight, if I remember to.

@bestander
Copy link
Member

ping, @KidkArolis, could you rebase?

Previously, when URLs got rewritten, the npm-registry would fail to find the right token in the config file.
@jhen0409
Copy link

jhen0409 commented Jan 2, 2017

Any update on this? It work for me! 👍

@bestander
Copy link
Member

@KidkArolis could you rebase once more please?

import {addSuffix, removePrefix} from '../util/misc';
import {YARN_REGISTRY} from '../constants.js';

const NPM_REGISTRY = /http[s]:\/\/registry.npmjs.org/g;

Choose a reason for hiding this comment

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

I think this should be https?, not http[s], right?

@bestander
Copy link
Member

Sorry for forgetting about this is it still relevant?

@bestander
Copy link
Member

As it is not in a mergeable state I am closing the PR.
Feel free to reopen a new one.

@bestander bestander closed this Apr 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants