Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

opts.env option to set script specific enviromnent variables #40

Open
wants to merge 2 commits into
base: latest
Choose a base branch
from

Conversation

olivr70
Copy link

@olivr70 olivr70 commented Aug 15, 2019

Added support for an env in the opts of makeEnv() (and therefore lifecycle()).

The original need is to set an environment variable with the path of the generated archive before invoking postpack script in npm/cli.

makeEnv() has a env parameter, but :

  • it ignores any variables beginning with "npm_"
  • it is not available in lifecycle()

The proposed implementation ignores all opts.env properties which are not of type "number" or "string".

Tests have been updated accordingly

@olivr70 olivr70 changed the title Pr env option opts.env option to set script specific enviromnent variables Aug 15, 2019
// assign script specific environnement variables (ignores non string values)
if (typeof opts.env === 'object') {
for (var k in opts.env) {
if (opts.env.hasOwnProperty(k)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.hasOwnProperty should never be called directly off of objects; what about an env var named hasOwnProperty (which is entirely valid)? Always do this:

Suggested change
if (opts.env.hasOwnProperty(k)) {
if (Object.prototype.hasOwnProperty.call(opts.env, k)) {

for (var k in opts.env) {
if (opts.env.hasOwnProperty(k)) {
var v = opts.env[k]
if (typeof v === 'string' || typeof v === 'number') {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason to ignore non-string/number values, instead of throwing?

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.

2 participants