jdx / npm-register

Your own private npm registry and backup server
ISC License
483 stars 68 forks source link

Converting start script to run with node so it works on windows and u… #103

Closed dgautsch closed 6 years ago

dgautsch commented 6 years ago

@Ianfeather and @jdxcode can you test this out and see if it works on your machines? It should resolve the issues we're having. Fixes #101

codecov[bot] commented 6 years ago

Codecov Report

Merging #103 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #103   +/-   ##
=======================================
  Coverage   89.63%   89.63%           
=======================================
  Files          27       27           
  Lines         434      434           
=======================================
  Hits          389      389           
  Misses         45       45

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 42b7420...d361187. Read the comment docs.

Ianfeather commented 6 years ago

Hey @dgautsch

Can confirm this works for running npm start within npm-register. 👍

It still doesn't fix the issue I have where I'm running npm-register directly. I have a container which starts up npm-register directly. In my package.json I have:

"scripts": {
    "start": "npm-register --always-https start"
},

This will throw the same error as in #101 regardless of the change in this PR. However I can make it work by updating that to:

"scripts": {
    "start": "node /node_modules/.bin/npm-register --always-https start"
},

Maybe my use case is a strange one? Either way I have a workaround, which people can now find in the issues here, so I don't think this is a huge problem. Still a bit of a mystery to me how it published with different line endings :/

Thanks for your help with this

dgautsch commented 6 years ago

Thanks @Ianfeather ! I'm glad we made a little progress. I'm going to clone the repo again and check the line endings and how git is configured and try republishing with this PR.

jdx commented 6 years ago

@dgautsch this isn't terribly well documented but you need to add a .cmd file for it to work on windows where you can install this with npm install -g npm-register. Just add a file like this:

https://github.com/dxcli/dev/blob/176fa573d8d783f9eb997c46b1d1ab5ac90376ee/bin/run.cmd#L1-L3

npm picks up a .cmd file automatically if it matches another bin file. In this case you'll have to replace run with npm-register.

dgautsch commented 6 years ago

Ah, that's good to know. I had no idea @jdxcode. Do you want me to revert this change? Seems it'll work as well.

jdx commented 6 years ago

no, your fix is good and solves a different but related problem. Your fix fixes running from WITHIN the app, my proposal fixes running from OUTSIDE