grunt-env icon indicating copy to clipboard operation
grunt-env copied to clipboard

Avoid setting undefined options

Open evanshortiss opened this issue 8 years ago • 4 comments

Ran into this bug in my own code. Basically I have two variations of a grunt task. One simulates a local environment, the other a DEV environment. In DEV and beyond an env var will be set, but in local it will not be set since it is generated by a platform I deploy my code on. This means I have this:

env: {
      options: {},
      local: {
        NOT_DEFINED_LOCALLY: function () {
          return grunt.config('isLocal') ? undefined : 'set';
        }
     }
}

The issue arises here when undefined is returned. Doing process.env.DEFINED_BASED_ON_ENV = undefined, works, but when we get it later it's a String like "undefined" since node.js seems to change it when being set! For example this check would pass:

// We're running locally so this should be undefined!
if (process.env.NOT_DEFINED_LOCALLY) {
  // Oops, we're in here since process.env.NOT_DEFINED_LOCALLY
  // was actually the String "undefined" and not just the undefined type
}

I made this PR since it works around this issue by allowing us to circumvent setting vars in certain configurations. We could have two options, e.g env.local and env.dev, but would be nice to avoid this pitfall in general.

evanshortiss avatar Nov 22 '16 18:11 evanshortiss

@jsoverson any thoughts on this?

evanshortiss avatar Nov 30 '16 14:11 evanshortiss

@evanshortiss certainly seems reasonable. Are you interested in adopting this project? I haven't been maintaining it for a very long time and would be glad to pass it on to someone who is still using it.

jsoverson avatar Nov 30 '16 19:11 jsoverson

I don't have many changes to make at present, but would certainly be happy to be a maintainer since we use it frequently on projects and might find new use cases to support 👍

evanshortiss avatar Nov 30 '16 21:11 evanshortiss

@jsoverson just a reminder on this one when you have the time

evanshortiss avatar Dec 16 '16 21:12 evanshortiss