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

npm #45 #48

Merged
merged 6 commits into from
Nov 13, 2018
Merged

npm #45 #48

merged 6 commits into from
Nov 13, 2018

Conversation

lil5
Copy link
Contributor

@lil5 lil5 commented Nov 3, 2018

https://www.npmjs.com/package/peer-calls

@jeremija I've given you write access to the npm package via your npm account: https://www.npmjs.com/~jeremija

@lil5 lil5 changed the title PR lil5/master npm #45 npm #45 Nov 3, 2018
@lil5
Copy link
Contributor Author

lil5 commented Nov 3, 2018

#45

@lil5
Copy link
Contributor Author

lil5 commented Nov 4, 2018

At the moment the npm readme#from-npm has a bad npm install instruction and cannot be updated until the next version is published,
see link for correct method.

@lil5 lil5 mentioned this pull request Nov 4, 2018
@jeremija
Copy link
Member

jeremija commented Nov 7, 2018

Hi @lil5,

Thanks for the effort! This looks good, although I wouldn't recommend the users to use --global flag since it pollutes the global node_modules, sometimes requires root access, and can cause security issues. Even webpack discourages it:

npm install --global webpack

Note that this is not a recommended practice. Installing globally locks you down to a specific version of webpack and could fail in projects that use a different version.

I'd write something like this instead:

mkdir peer-calls
cd peer-calls

npm install peer-calls

npx peer-calls --NODE_CONFIG={"baseUrl":"",iceServers:[...]}
# or
NODE_CONFIG=$(cat node_modules/peer-calls/config/default.json) npx peer-calls

@lil5
Copy link
Contributor Author

lil5 commented Nov 7, 2018

Note that this is not a recommended practice. Installing globally locks you down to a specific version of webpack and could fail in projects that use a different version

That's only a problem when running multiple instances on the same os, running different versions, not really a usual use case for peercalls imho

Upgrades are still possible in global npm

npm up --global peer-calls

@lil5
Copy link
Contributor Author

lil5 commented Nov 7, 2018

sometimes requires root access, and can cause security issues.

True installing and updating will then require root access, will change it.
Edit:

  • Looking at npm documentation npm install --global <> does not necessarily require root link see prefix
  • And as the docs explain the global install is there to run programs in terminal.

Running it will not, you could even run in different user: sudo -uwww-data peercalls

@lil5
Copy link
Contributor Author

lil5 commented Nov 7, 2018

npx

I don't understand why one would need npx, npm start(in peer-calls dir) does exactly the same thing.

[lil5@lil5-230 peer-calls]$ npm start

> peer-calls@2.0.3 start /home/lil5/Downloads/peer-calls
> node src/index.js

  peercalls WebSocket URL: /ws +0ms
  peercalls Listening on: 3000 +73ms
^C  peercalls Closing server... +6s
  peercalls Bye! +2ms

@jeremija
Copy link
Member

jeremija commented Nov 8, 2018

The npx binary runs a binary from ./node_modules/.bin. The npx peer-calls command is basically a shorthand for ./node_modules/.bin/peer-calls, and npm has been automatically installing it since v5.2.0.

You would only need to run npx if you install peer-calls as a module via npm, not if you clone the repository and build it yourself.

  • Looking at npm documentation npm install --global <> does not necessarily require root link see prefix

True, but most users won't have that configured and do not understand the implications of installing third party packages as root. One the other hand, complex installation instructions could prevent the users from trying out this package 🙂

Whenever I need a global package, I just run the npm install in the ~/opt folder and I have the ~/opt/node_modules/.bin added to my PATH variable.

  • And as the docs explain the global install is there to run programs in terminal.

Correct. But this is coming from my Linux sysadmin experience and I do not like running things as root when they do not absolutely require it. There are also a lot of articles/discussions about this very issue 1, 2, 3, 4.

@lil5
Copy link
Contributor Author

lil5 commented Nov 8, 2018

Thanks for the read ;)

Here is an interesting npm issue

One the other hand, complex installation instructions could prevent the users from trying out this package.

This is an important point. I suggest adding a notice warning not to use --global in server environments?

@lil5
Copy link
Contributor Author

lil5 commented Nov 9, 2018

How to prevent permissions errors | npm Documentation - https://docs.npmjs.com/getting-started/fixing-npm-permissions

@lil5
Copy link
Contributor Author

lil5 commented Nov 13, 2018

@jeremija I've noticed you've published on npm

Close PR? Or could this still be merged, if so what's your view, having discussed npm global?

@jeremija jeremija merged commit db0c6a2 into peer-calls:master Nov 13, 2018
@lil5
Copy link
Contributor Author

lil5 commented Nov 14, 2018

Thanks for the merge 🎊

@lil5 lil5 deleted the lil5/master-npm branch November 14, 2018 11:24
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.

None yet

2 participants