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

Fixed linter on windows machines #22

Merged
merged 2 commits into from
Jul 23, 2015
Merged

Fixed linter on windows machines #22

merged 2 commits into from
Jul 23, 2015

Conversation

benjaminRomano
Copy link
Contributor

Passing in custom options without process.env.OS to fix issues on windows machines.

I'm creating a copy of the process.env by using JSON.parse(JSON.stringify(process.env)), which is a bit ugly. I can change that if you know of a cleaner way to do it.

@@ -31,7 +31,9 @@ module.exports = Helpers =
else
resolve(data.stderr.join(''))
if isNodeExecutable
spawnedProcess = new BufferedNodeProcess({command, args, stdout, stderr, exit})
options.env = JSON.parse(JSON.stringify(process.env))
Copy link
Owner

Choose a reason for hiding this comment

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

How about options.env ?= Object.create(process.env)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw that in the bufferedNodeProcess code, but for some reason, Object.create(process.env) returns an empty object when I use it here.

Copy link
Owner

Choose a reason for hiding this comment

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

It doesn't return an empty object, it just moves stuff to the prototype. You might not see it in the debugger at top level but it's still there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow can't believe I didn't know that... So I looked more into Object.create and an interesting thing about it is that the properties created are not writable, enumerable or configurable by default

@steelbrain
Copy link
Owner

I am so glad that you found the key which was causing the problem. Thanks 👏

@steelbrain steelbrain self-assigned this Jul 23, 2015
@steelbrain
Copy link
Owner

LGTM, all I need now is some users confirming that this works. I'll post this in the jshint thread.

@steelbrain
Copy link
Owner

Thanks again 👏

steelbrain added a commit that referenced this pull request Jul 23, 2015
Fixed linter on windows machines
@steelbrain steelbrain merged commit 1965d08 into steelbrain:master Jul 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants