Remove node_modules from .cfignore #8

Closed
dotchev opened this Issue Sep 28, 2015 · 5 comments

Comments

Projects
None yet
2 participants
@dotchev
Contributor

dotchev commented Sep 28, 2015

There are several good reasons to avoid npm install during productive use.
Currently npm does not guarantee that you will get the same (bit for bit) dependencies as during your tests. So this results in the following risks:

  • Security: you cannot verify that malicious code is not injects in some of your dependencies
  • Stability: the content of some dependency could change without changing its version or even npm registry could be down

So it should be possible to push an app with pre-downloaded dependencies, i.e. with node_modules.
Currently this is not possible as .cfignore skips node_modules from cf push.

@pmuellr

This comment has been minimized.

Show comment
Hide comment
@pmuellr

pmuellr Sep 28, 2015

Member

It's true that there are good reasons to avoid npm install. However, there are also good reasons to use npm install. Is there a way we can support both scenarios?

I think if you're going to remove node_modules from .cfignore, you prolly want to remove it from .gitignore as well.

Perhaps the best story for folks who want a customized version, is to fork the project, change whatever you want, and then in your package.json, reference cfenv via a git url.

Member

pmuellr commented Sep 28, 2015

It's true that there are good reasons to avoid npm install. However, there are also good reasons to use npm install. Is there a way we can support both scenarios?

I think if you're going to remove node_modules from .cfignore, you prolly want to remove it from .gitignore as well.

Perhaps the best story for folks who want a customized version, is to fork the project, change whatever you want, and then in your package.json, reference cfenv via a git url.

@dotchev

This comment has been minimized.

Show comment
Hide comment
@dotchev

dotchev Sep 28, 2015

Contributor

Some people do commit node_modules in git, others download them during build and some run npm install during deployment (e.g. cf push).
See http://www.letscodejavascript.com/v3/blog/2014/03/the_npm_debacle.

In any case it should be up to the application to decide. A dependency should not force a particular process. One can easily add/remove node_modules in .cfignore and/or .gitignore at the root of their app.

Contributor

dotchev commented Sep 28, 2015

Some people do commit node_modules in git, others download them during build and some run npm install during deployment (e.g. cf push).
See http://www.letscodejavascript.com/v3/blog/2014/03/the_npm_debacle.

In any case it should be up to the application to decide. A dependency should not force a particular process. One can easily add/remove node_modules in .cfignore and/or .gitignore at the root of their app.

@pmuellr

This comment has been minimized.

Show comment
Hide comment
@pmuellr

pmuellr Sep 29, 2015

Member

So, looking at this again, I think you're right. Would you like to send a PR with the fixed .cfignore file?

Member

pmuellr commented Sep 29, 2015

So, looking at this again, I think you're right. Would you like to send a PR with the fixed .cfignore file?

@pmuellr pmuellr closed this in 960ca93 Sep 29, 2015

pmuellr added a commit that referenced this issue Sep 29, 2015

Merge pull request #9 from dotchev/patch-1
Fix #8 - Remove node_modules from .cfignore
@pmuellr

This comment has been minimized.

Show comment
Hide comment
@pmuellr

pmuellr Sep 29, 2015

Member

@dotchev - I updated npm to a 1.0.1 with your fix. Please test and post results back here.

Member

pmuellr commented Sep 29, 2015

@dotchev - I updated npm to a 1.0.1 with your fix. Please test and post results back here.

@pmuellr pmuellr reopened this Sep 29, 2015

@pmuellr

This comment has been minimized.

Show comment
Hide comment
@pmuellr

pmuellr Oct 9, 2015

Member

closing as I believe this has been resolved

Member

pmuellr commented Oct 9, 2015

closing as I believe this has been resolved

@pmuellr pmuellr closed this Oct 9, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment