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: creating of enabledGlobals object without prototype (fixes #11929) #11935

Merged
merged 2 commits into from Jul 6, 2019

Conversation

@finico
Copy link
Contributor

commented Jul 3, 2019

What is the purpose of this pull request? (put an "X" next to item)

[x] Bug fix

issue: #11929

What changes did you make? (Give an overview)

There was an issue when there were comments like /*global toString:true*/
Therefor if (enabledGlobals[id])

if (enabledGlobals[id]) {

was true because of Object.prototype.toString

finico

@eslint eslint bot added the triage label Jul 3, 2019

finico

@platinumazure platinumazure self-requested a review Jul 3, 2019

@platinumazure platinumazure added accepted bug core and removed triage labels Jul 3, 2019

@platinumazure

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

Hi @finico, thanks for the PR. Nice find!

@platinumazure
Copy link
Member

left a comment

LGTM, thanks! Would like another set of eyes on this.

@aladdin-add
Copy link
Member

left a comment

LGTM, thanks!
just a reminder: you don't have to specify globlas like toString: technically it's a global. if not defined, it will try to find global.toString => Object.prototype.toString (since global is an object, its prototype is on object.prototype )

@finico

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

@aladdin-add I agree, but it is a case from linked issue. I just wanted to fix it.

@aladdin-add aladdin-add changed the title Fix: creating of enabledGlobals object without prototype Fix: creating of enabledGlobals object without prototype (fixes #11929) Jul 3, 2019

@aladdin-add aladdin-add merged commit 1fb3620 into eslint:master Jul 6, 2019

9 checks passed

commit-message PR title follows commit message guidelines
Details
continuous-integration Build #20190703.3 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@aladdin-add

This comment has been minimized.

Copy link
Member

commented Jul 6, 2019

merged, thanks for contributing! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.