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

fix #467 #590

Closed
wants to merge 2 commits into from
Closed

fix #467 #590

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 13, 2018

No description provided.

@coveralls
Copy link

coveralls commented Jul 13, 2018

Coverage Status

Coverage increased (+1.4%) to 75.152% when pulling 690e365 on errorx666:issue-467 into 05b0ceb on visionmedia:master.

Copy link
Member

@Qix- Qix- left a comment

Choose a reason for hiding this comment

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

This seems entirely innocuous. How does this fix anything?

@Qix- Qix- added bug This issue identifies a malfunction change-minor This proposes or provides a change that requires a minor release awaiting-response This issue or pull request is awaiting a user's response labels Jul 14, 2018
@ghost
Copy link
Author

ghost commented Jul 14, 2018

As stated in #467, if using webpack's DefinePlugin will rewrite process.env.DEBUG to a literal true or false.

The function becomes:

function save(namespaces) {
  if (null == namespaces) {
    // If you set a process.env field to null or undefined, it gets cast to the
    // string 'null' or 'undefined'. Just delete instead.
    delete true; // syntax error
  } else {
    true = namespaces; // syntax error
  }
}

With this patch, we avoid assigning to an r-value.

This is currently blocking me from using the bcrypt library.

@Qix-
Copy link
Member

Qix- commented Jul 15, 2018

Please put all of that into a comment, and don't alias as it's confusing (I thought the aliasing fixed something bizarre, hence my previous comment).

@ghost
Copy link
Author

ghost commented Jul 15, 2018

I added the comment.

@Qix- Qix- removed the awaiting-response This issue or pull request is awaiting a user's response label Dec 13, 2018
@Qix-
Copy link
Member

Qix- commented Dec 13, 2018

Sorry it took me so long to get to this. Any chance you could rebase off master?

@Qix- Qix- added the awaiting-response This issue or pull request is awaiting a user's response label Dec 13, 2018
@Qix-
Copy link
Member

Qix- commented Dec 19, 2018

Actually, I've changed my mind on this - please see #467 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-response This issue or pull request is awaiting a user's response bug This issue identifies a malfunction change-minor This proposes or provides a change that requires a minor release
Development

Successfully merging this pull request may close these issues.

None yet

3 participants