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

Avoid setting undefined options #41

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

evanshortiss
Copy link

@evanshortiss evanshortiss commented Nov 22, 2016

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.

@@ -29,6 +29,7 @@
},
"devDependencies": {
"grunt": "~0.4.5",
"grunt-cli": "~1.2.0",
Copy link
Author

@evanshortiss evanshortiss Nov 22, 2016

Choose a reason for hiding this comment

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

npm test only works if grunt-cli is a global install. This fixes that.

@evanshortiss evanshortiss changed the title Avoid setting undefined options since Avoid setting undefined options Nov 23, 2016
@evanshortiss
Copy link
Author

@jsoverson any thoughts on this?

@jsoverson
Copy link
Owner

@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.

@evanshortiss
Copy link
Author

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
Copy link
Author

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

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.

2 participants