[new-rule] no-restricted-globals

[Zzzen]

PR checklist

• Addresses an existing issue: New rule to forbid access to commonly-confused ambient globals #922
• New feature, bugfix, or enhancement
  • Includes tests
• Documentation update

Overview of change:

prevents accidental access to variables that are declared in lib.dom.d.ts.

Is there anything you'd like reviewers to focus on?

1. a descriptive rule name
2. corner cases
3. performance

CHANGELOG.md entry:

[palantirtech]

Thanks for your interest in palantir/tslint, @Zzzen! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

[JoshuaKGoldberg]

Could you add a rationale explaining why this rule is useful? It would be confusing as a newcomer to TSLint reading the rule page on palantir.github.io/tslint why this would be useful.

It should probably also recommend specifically not using the dom lib in your tsconfig if you're not writing a browser app.

[Zzzen]

Haven't you check out New rule to forbid access to commonly-confused ambient globals? I write browser app primarily and have been bit by name in lib.dom.d.ts. Maybe I should rename this to forbid-ambient-globals and provides options like

[
  {
    "file": "lib.dom.d.ts",
    "variables": [
      "event",
      "name"
    ]
  },
  {
    "file": "@types/jquery/index.d.ts",
    "variables": [
      "$"
    ]
  }
]

[Zzzen]

Definition file names are redundant since we cannot use a global declared in lib A if it is also declared and used in lib B.

[Zzzen]

@JoshuaKGoldberg any advice? thanks

[JoshuaKGoldberg]

This seems like a rather difficult problem. Very cool and absolutely a problem that needs to be dealt with, but needs some more thought into best implementation details.

Can you file a separate issue to discuss continue this conversation in the linked issue I absolutely didn't miss on the first readthrough?

[JoshuaKGoldberg]

Nit: escape the "event" in "global variable event" with code to make it a little more clear.

global variable \`event\`, but using

[JoshuaKGoldberg]

Nit: just "considered a bad practice"

[JoshuaKGoldberg]

Or a local variable, right?

Use a local parameter or variable instead.

[Zzzen]

Updated. thanks

[JoshuaKGoldberg]

What if you declare something in an externals.d.ts file, or define your own lib.dom.d.ts with either that name or my-lib.d.ts?

Or what about a project that uses namespaces and has a const name = "HA!"; somewhere?

Maybe the rule should get the list of files TS is compiling with (can we do that?) along with its compiler options, and add lib.d.ts-like files as per lib and --noLib compiler settings?
Maybe the rule should keep a default list of regular expressions to match names against, like lib*.d.ts?

[Zzzen]

IMO, custom definition files should be considered the same as official lib.dom.d.ts. There is no need to get compiler options.

[aervin]

@Zzzen this is awesome! The ability to add code examples to the documentation just landed on master--would you mind adding some for this rule? See the curly rule for an example.

[Zzzen]

*.d.ts is working now. But as you can see from CI checks, something went wrong with namespace: name inside namespace/lib.ts.lint is resolved to lib.dom.d.ts rather than ./core.d.ts.
lib.ts.lint
core.d.ts

Definition files under tests do not seem to be included in compilation. Because compiler cannot find declaration of myGlobal in test/rules/no-restricted-globals/lib-dom/test.ts.lint though I've declared it in ./custom-global.d.ts.lint. Same goes to test/rules/no-restricted-globals/namespace/lib.ts.lint. What did I do wrong?

test.ts.lint
custom-global.d.ts.lint

[rafeememon]

Hey all, any word on when this PR can be merged? I've been bitten by this issue a handful of times and would love to use this rule. Thanks!

[Zzzen]

Please take a look when you have time. @JoshuaKGoldberg

[JoshuaKGoldberg]

Heya - I can take a look and give feedback, but I'm not actually an employee of Palantir or on the TSLint maintenance team. So you might want to ping one of them. Sorry!

[Zzzen]

😂
ping @giladgray

[JoshuaKGoldberg]

Has the "Some more test cases to consider..." comment been addressed?

[jdom]

Would love to see this in too, as I'm currently re-bundling updated lib declaration files just to ban a few of these global variables (which is very cumbersome)... Just to confirm, this is blocked only on the author providing more test cases, but the behavior/naming is acceptable?

[JoshuaKGoldberg]

@jdom also pending review from a member of Palantir, yes.

Edit 1/2/2019 - realized some more changes should be made, sorry!

[bradleyayers]

This would be very useful, is there anything that can be done to move it along?

[Zzzen]

🤔 I guess all requested changes already have been addressed?

[JoshuaKGoldberg]

Had time to take a deeper look - a few things to change.

This should still be reviewed by a reviewer from palantirtech.

[Zzzen]

Code updated. You're really helpful. @JoshuaKGoldberg

[adidahiya]

nice work @Zzzen

[sandersn]

This errors too often if the declaration comes from a declaration file that is not lib.dom.d.ts. Consider the case of adone's types in Definitely Typed:

// @Filename: adone-tests.ts
namespace eventsTests {
    namespace EventEmitter {
        namespace static {
            const a: number = adone.event.Emitter.listenerCount(new adone.event.Emitter(), "event");
            const b: number = adone.event.Emitter.defaultMaxListeners;
        }
// @Filename: events.d.ts
declare namespace adone {
    namespace event {
        class Emitter {
            static listenerCount(emitter: Emitter, event: string | symbol): number;
            static defaultMaxListeners: number;
            // ...

You will get bogus errors on event.