Skip to content
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

Support codeload.github.com packages #9

Closed
skeggse opened this issue Dec 13, 2017 · 3 comments
Closed

Support codeload.github.com packages #9

skeggse opened this issue Dec 13, 2017 · 3 comments

Comments

@skeggse
Copy link
Contributor

skeggse commented Dec 13, 2017

Issue description:

I'm seeing the following when trying to run synp against one of our yarn.lock files:

$ synp
Invalid hex string

  Usage: synp [options]


  Options:

    -V, --version                    output the version number
    -s, --source-file [source-file]  The path to the yarn.lock or package-lock.json to be converted
    -h, --help                       output usage information

After digging in a bit, I noticed that it's failing on an entry that looks like this:

throng@mixmaxhq/throng#eb_support:
  version "4.0.0"
  resolved "https://codeload.github.com/mixmaxhq/throng/tar.gz/8a015a378c2c0db0c760b2147b2468a1c1e86edf"
  dependencies:
    lodash.defaults "^4.0.1"

It looks like this would need explicit support.

EDIT: it looks like something similar happens going from npm to yarn, with this npm lockfile entry:

"throng": {
  "version": "github:mixmaxhq/throng#8a015a378c2c0db0c760b2147b2468a1c1e86edf",
  "requires": {
    "lodash.defaults": "4.2.0"
  }
}

which produces this error:

TypeError: Cannot read property 'replace' of undefined
    at npmToYarnResolved (/.../node_modules/synp/lib/entry.js:17:28)
    at yarnEntry (/.../node_modules/synp/lib/entry.js:48:31)
    at Object.keys.reduce (/.../node_modules/synp/lib/tree.js:23:23)
    at Array.reduce (native)
    at buildYarnTree (/.../node_modules/synp/lib/tree.js:18:47)
    at npmToYarn (/.../node_modules/synp/index.js:35:22)
    at run (/.../node_modules/synp/cli/run.js:25:20)
    at Object.<anonymous> (/.../node_modules/synp/cli/synp.js:13:1)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
@imsnif
Copy link
Owner

imsnif commented Dec 14, 2017

Hey @skeggse - thanks so much for finding this issue and submitting a fix!
I think for now the best thing would be to keep the yarn.lock => package-lock.json test+fix and work on #5 as you mentioned for the package-lock.json => yarn.lock issue.

Regarding the registry url: I don't think there is a need to translate yarnpkg to npmjs and the other way around. synp translates a file as-is, including the registry (I actually think doing anything else would be too surprising!) - so what I did in the other tests is simply edit the URL manually in the fixtures to make the test pass. The same can be done here.

I'd be happy to merge both PRs, but I do not have permissions to push these adjustments to your branch.
So, if you could either allow edits or make these yourself and reissue the PRs (preferably as a single one) it would be great.

@skeggse
Copy link
Contributor Author

skeggse commented Dec 14, 2017

but I do not have permissions to push these adjustments to your branch.

Oh, that's funny - I figured this option would allow that:

image

I went ahead and updated #11 to include the corresponding test from #10. I'm on wonderful hotel wifi, so these changes are a bit slow atm.

@imsnif
Copy link
Owner

imsnif commented Dec 14, 2017

Alright, I merged #11 and bumped the version. I have similar wifi issues as I am on a plane now, so might only get to publish to npm later today.
I opened another issue regarding supporting this the other way.
Thanks for your contribution, @skeggse! Very much appreciated.

(Regarding the permissions - it is quite odd, I agree! Perhaps it's because the fork is part of an organization and some permissions there get in the way? Just guessing...)

@imsnif imsnif closed this as completed Dec 14, 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

No branches or pull requests

2 participants