Skip to content
This repository was archived by the owner on Aug 29, 2018. It is now read-only.

NodeJS Cartridge Should Only Pull Production Modules #5628

Closed
wants to merge 1 commit into from
Closed

NodeJS Cartridge Should Only Pull Production Modules #5628

wants to merge 1 commit into from

Conversation

kevinconaway
Copy link
Contributor

Issue #5627

@detiber
Copy link
Contributor

detiber commented Jul 18, 2014

Would it make sense to allow this to be user configurable?

You could check for the existence of either a marker file or an environment variable to determine whether or not to add the --production flag.

@kevinconaway
Copy link
Contributor Author

Not in my opinion. The devDependencies are used for build/test, not actually running the app.

That said, having it configurable would open up possibilities that I'm not aware of.

@bparees
Copy link
Contributor

bparees commented Aug 1, 2014

I think we want this to be configurable. We particularly don't want to risk breaking existing user applications who might be inadvertently expecting dev dependencies to be installed. I've opened a trello card to track this feature request:
https://trello.com/c/hDNn3MmU/260-support-npm-production-mode

but if you want to modify this PR to based the --production flag on an env variable (with a default to not pass --production, for backwards compatibility), that'd be great!

@kevinconaway
Copy link
Contributor Author

Thanks Ben, will do. Is this something that the end user would set with rhc set-env? I'm not familiar with how the app author would expose an envvar to the cartridge.

@bparees
Copy link
Contributor

bparees commented Aug 1, 2014

yeah, rhc set-env, or passing -e on app create to set it from the start. they could also create a pre-build hook that set it.

@danmcp
Copy link
Contributor

danmcp commented Aug 27, 2014

Dup of #5215

@danmcp danmcp closed this Aug 27, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants