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

Support Node.js 0.10 (Revert #1059) #1074

Merged
merged 1 commit into from
Oct 11, 2019

Conversation

abetomo
Copy link
Collaborator

@abetomo abetomo commented Oct 11, 2019

Pull Request

Problem

#1073

Solution

Revert #1059

ChangeLog

@abetomo abetomo requested a review from shadowspawn October 11, 2019 03:14
index.js Outdated
@@ -484,7 +484,7 @@ Command.prototype.parse = function(argv) {
})[0];
}

if (this._execs.has(name)) {
if (this._execs[name]) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously it was the following code.

if (this._execs[name] && typeof this._execs[name] !== 'function') {

The function check was deleted because it was a code when it was an Array.
reference: #249

> a = []; a['filter']
[Function: filter]
> o = {}; o['filter']
undefined

Copy link
Collaborator

Choose a reason for hiding this comment

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

The function check is to avoid treating built-in functions like toString as though they are subcommands, so I recommend a revert to the full previous code.

(The problem the Set was fixing was correctly handling a built-in property that is not a function, which the old code gets wrong.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about === true?

> o = {}; o['toString'] === true
false

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was just thinking something similar! Yes. Simple and elegant.

@abetomo
Copy link
Collaborator Author

abetomo commented Oct 11, 2019

test failed: #1059 (comment)

@abetomo abetomo force-pushed the support_node0.10_in_v2 branch from 931e382 to 38b29eb Compare October 11, 2019 03:37
Copy link
Collaborator

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

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

Thanks!

@abetomo
Copy link
Collaborator Author

abetomo commented Oct 11, 2019

I'll do npm publish.

@abetomo abetomo merged commit a591f87 into tj:release/2.x Oct 11, 2019
@abetomo abetomo deleted the support_node0.10_in_v2 branch October 11, 2019 03:59
@shadowspawn
Copy link
Collaborator

Thanks Abetomo. (I think I used publish tag 2_x with the previous 2.x publish, but then deleted it after publish. No need to delete it though. The publish tag needs to look different than a version number because they share same namespace.)

@abetomo
Copy link
Collaborator Author

abetomo commented Oct 11, 2019

I'm sorry!
I was impatient and unpublished 2.20.2.

If you unpublish, you cannot publish again with that version.
Will you release in 2.20.3?

@shadowspawn
Copy link
Collaborator

shadowspawn commented Oct 11, 2019

Bumping it to 2.20.3 is fine. (No worries.)

@shadowspawn
Copy link
Collaborator

This issue was responsibly reported by the Checkmarx Application Security Research Team. It was fixed in v2.20.3 (#1074) and v3.0.2(#1056).

The Checkmarx vulnerability library lists this as: https://devhub.checkmarx.com/cve-details/Cx435a6fda-ca38/

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