Update: support globalThis (refs #12670) #12774

yeonjuan · 2020-01-11T07:36:59Z

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

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[x] Add something to the core
[x] Other, please explain:

This PR is for supporting globalThis in eslint.

What changes did you make? (Give an overview)
Is there anything you'd like reviewers to focus on?

To Note
This PR upgrade eslint-utils to 2.0.0 (d3847f2) and add globalThis in environments es2020(c0357bc).

By upgrade eslint-utils, some rules seem to supports globalThis. So I just added test cases for globalThis.
- require-unicode-regexp
- prefer-regex-literals
- prefer-object-spread
- prefer-named-capture-group
- prefer-exponentiation-operator
- no-misleading-character-class
- no-redeclare
Even upgrade eslint-utils, some rules need to be changed for recognizing globalThis. So did it.
- no-alert
- no-eval
- no-obj-calls
no-implied-eval was not designed to consider global reference so I changed it in another PR( Update: consider env in no-implied-eval (fixes #12733) #12757).
- no-implied-eval ( blocked by Update: consider env in no-implied-eval (fixes #12733) #12757 )

mdjermanovic · 2020-01-11T16:03:37Z

lib/rules/no-obj-calls.js

@@ -54,7 +67,7 @@ module.exports = {
                }

                for (const { node } of tracker.iterateGlobalReferences(traceMap)) {
-                    context.report({ node, messageId: "unexpectedCall", data: { name: node.callee.name } });
+                    context.report({ node, messageId: "unexpectedCall", data: { name: getReportNodeName(node) } });


Thanks for fixing this bug!

Current behavior (demo):

/* eslint no-obj-calls: "error"*/ /* eslint-env browser*/ window.JSON(); // 'undefined' is not a function (no-obj-calls)

A question, maybe we could use path from iterateglobalreferences?

I guess it depends on whether we want to report "JSON is not a function" or "foo is not a function" in cases such as the following:

/* eslint no-obj-calls: "error"*/ var foo = bar ? baz : JSON; foo();

Ok, I'll try it! :) But what would be a better report?
In my personal opinion, current behavior looks better. But I respect what eslint members think

I'm not sure, let's wait for more opinions.

Current behavior reports "foo is not a function".

I'd vote for "JSON is not a function" as it might be a bit more helpful for the user to find out what's actually happening and why is the location of "foo()" reported as a global object call.

I'm not sure, let's wait for more opinions.

Ok. And just to note.
The reason for my preference was that foo is not a function looks like a more intuitive explanation for the code.

I'd vote for "JSON is not a function" as it might be a bit more helpful for the user to find out what's actually happening and why is the location of "foo()" reported as a global object call.

But I agree that JSON is not a function is more helpful to find out what's actually happening. it seems more reasonable 👍

Could we do both? "foo is a reference to JSON, which is not a function"? That seems like it gives the right amount of information without being too confusing or requiring you to figure out what the code is doing.

Could we do both? "foo is a reference to JSON, which is not a function"?

That would be ideal!

I'd be also fine with "foo is not a function" in this PR (it's already much better than "undefined is not a function") and improving the message further in another PR.

Could we do both? "foo is a reference to JSON, which is not a function"?

Agree! it would make the message more clear. maybe it needs new messageId like -unexpectedRefCall. I will do it. thanks :)

# Conflicts: # tests/lib/rules/prefer-object-spread.js

# Conflicts: # tests/lib/rules/no-eval.js

ljharb · 2020-02-27T15:39:13Z

fwiw i don’t see how this is breaking; if it would have broken anybody, then the name “globalThis” wouldn’t have been web compatible to add to the language. Because it was added, nobody could possibly be broken by this change who wasn’t already broken by every JS engine.

mdjermanovic · 2020-03-12T21:37:16Z

This PR is blocked by #12757?

In particular, waiting for #12757 to be merged in order to add something to no-implied-eval?

btmills

LGTM. Thank you for updating all these rules! I particularly like the reporting improvements 👍

nzakas

Awesome, thanks!

kaicataldo · 2020-03-17T17:53:41Z

lib/rules/no-obj-calls.js

@@ -35,7 +48,8 @@ module.exports = {
        schema: [],

        messages: {
-            unexpectedCall: "'{{name}}' is not a function."
+            unexpectedCall: "'{{name}}' is not a function.",
+            unexpectedRefCall: "'{{name}}' is reference to '{{ref}}', which is not a function."


kaicataldo

Thank you for working on this!

* support globalThis in no-alert (ref eslint#12670) * support globalThis in no-eval (ref eslint#12670) * bump eslint-utils to ^2.0.0 (ref eslint#12670) * add globalThis test cases in require-unicode-regexp (ref eslint#12670) * add globalThis test cases in prefer-regex-literals (ref eslint#12670) * add globalThis test cases in prefer-object-spread (ref eslint#12670) * add globalThis test cases in prefer-named-capture-group (ref eslint#12670) * add globalThis test cases in prefer-exponentiation-operator (ref eslint#12670) * add globalThis test cases in no-misleading-character-class (ref eslint#12670) * edit test cases in no-eval * support globalThis in no-obj-calls (ref eslint#12670) * add globalThis test cases in no-redeclare (eslint#12670) * change to use getPropertyName * fix tpo * add messageID - unexpectedRefCall

yeonjuan added 13 commits December 28, 2019 00:16

support globalThis in no-alert (ref #12670)

c0357bc

support globalThis in no-eval (ref #12670)

2db8bec

bump eslint-utils to ^2.0.0 (ref #12670)

d3847f2

add globalThis test cases in require-unicode-regexp (ref #12670)

a5a6504

add globalThis test cases in prefer-regex-literals (ref #12670)

b2db2b5

add globalThis test cases in prefer-object-spread (ref #12670)

4252206

add globalThis test cases in prefer-named-capture-group (ref #12670)

dd669b7

add globalThis test cases in prefer-exponentiation-operator (ref #12670)

069f1be

add globalThis test cases in no-misleading-character-class (ref #12670)

0621dc6

edit test cases in no-eval

cbc26e7

merge upstream

1847241

support globalThis in no-obj-calls (ref #12670)

130c1c3

add globalThis test cases in no-redeclare (#12670)

771e4bc

eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Jan 11, 2020

yeonjuan changed the title ~~Breaking: support globalThis (ref #12670)~~ Breaking: support globalThis (refs #12670) Jan 11, 2020

yeonjuan added 2 commits January 11, 2020 17:03

change to use getPropertyName

309b019

fix tpo

1361436

mdjermanovic mentioned this pull request Jan 11, 2020

Support globalThis #12670

Closed

mdjermanovic reviewed Jan 11, 2020

View reviewed changes

mysticatea added this to Implemented, pending review in v7.0.0 Jan 14, 2020

kaicataldo mentioned this pull request Jan 15, 2020

Update: consider env in no-implied-eval (fixes #12733) #12757

Merged

yeonjuan added 2 commits January 19, 2020 12:22

merge upstream

b469eaa

# Conflicts: # tests/lib/rules/prefer-object-spread.js

merge upstream

116517d

# Conflicts: # tests/lib/rules/no-eval.js

add messageID - unexpectedRefCall

4b41842

btmills approved these changes Mar 15, 2020

View reviewed changes

nzakas approved these changes Mar 16, 2020

View reviewed changes

kaicataldo reviewed Mar 17, 2020

View reviewed changes

kaicataldo approved these changes Mar 17, 2020

View reviewed changes

kaicataldo changed the title ~~Breaking: support globalThis (refs #12670)~~ Update: support globalThis (refs #12670) Mar 17, 2020

kaicataldo added enhancement This change enhances an existing feature of ESLint and removed breaking This change is backwards-incompatible labels Mar 17, 2020

kaicataldo merged commit 183e300 into eslint:master Mar 17, 2020

v7.0.0 automation moved this from Implemented, pending review to Done Mar 17, 2020

mdjermanovic mentioned this pull request Mar 22, 2020

Update: Report constructor calls in no-obj-calls #12909

Merged

2 tasks

eslint-deprecated bot locked and limited conversation to collaborators Sep 15, 2020

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Sep 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: support globalThis (refs #12670) #12774

Update: support globalThis (refs #12670) #12774

yeonjuan commented Jan 11, 2020 •

edited

mdjermanovic Jan 11, 2020

mdjermanovic Jan 11, 2020

yeonjuan Jan 11, 2020 •

edited

mdjermanovic Jan 11, 2020

yeonjuan Jan 11, 2020 •

edited

nzakas Mar 12, 2020

mdjermanovic Mar 12, 2020

yeonjuan Mar 13, 2020

ljharb commented Feb 27, 2020

mdjermanovic commented Mar 12, 2020

btmills left a comment

nzakas left a comment

kaicataldo Mar 17, 2020

kaicataldo left a comment

Update: support globalThis (refs #12670) #12774

Update: support globalThis (refs #12670) #12774

Conversation

yeonjuan commented Jan 11, 2020 • edited

mdjermanovic Jan 11, 2020

Choose a reason for hiding this comment

mdjermanovic Jan 11, 2020

Choose a reason for hiding this comment

yeonjuan Jan 11, 2020 • edited

Choose a reason for hiding this comment

mdjermanovic Jan 11, 2020

Choose a reason for hiding this comment

yeonjuan Jan 11, 2020 • edited

Choose a reason for hiding this comment

nzakas Mar 12, 2020

Choose a reason for hiding this comment

mdjermanovic Mar 12, 2020

Choose a reason for hiding this comment

yeonjuan Mar 13, 2020

Choose a reason for hiding this comment

ljharb commented Feb 27, 2020

mdjermanovic commented Mar 12, 2020

btmills left a comment

Choose a reason for hiding this comment

nzakas left a comment

Choose a reason for hiding this comment

kaicataldo Mar 17, 2020

Choose a reason for hiding this comment

kaicataldo left a comment

Choose a reason for hiding this comment

yeonjuan commented Jan 11, 2020 •

edited

yeonjuan Jan 11, 2020 •

edited

yeonjuan Jan 11, 2020 •

edited