feat: update rule no-restricted-browser-globals-during-ssr to refle…

…ct best practices @W-14336890
salesforce · Oct 20, 2023 · a75ef1d · a75ef1d
1 parent c5ef560
commit a75ef1d
Show file tree

Hide file tree

Showing 5 changed files with 262 additions and 16 deletions.
diff --git a/docs/rules/no-restricted-browser-globals-during-ssr.md b/docs/rules/no-restricted-browser-globals-during-ssr.md
@@ -19,11 +19,52 @@ export default class Foo extends LightningElement {
 }
 ```
 
+```js
+import { LightningElement } from 'lwc';
+
+export default class Foo extends LightningElement {
+    constructor() {
+        this.handleResize = this.handleResize.bind(this);
+        this.handleClick = this.handleClick.bind(this);
+    }
+
+    connectedCallback() {
+        globalThis.addEventListener('resize', this.handleResize);
+        document.addEventListener('click', this.handleClick);
+    }
+
+    disconnectedCallback() {
+        globalThis.removeEventListener('resize', this.handleResize);
+        document.removeEventListener('click', this.handleClick);
+    }
+
+    handleResize(event) {
+        /* ... */
+    }
+
+    handleClick(event) {
+        /* ... */
+    }
+}
+```
+
 Examples of **correct** code for this rule:
 
 ```js
 import { LightningElement } from 'lwc';
 
+export default class Foo extends LightningElement {
+    connectedCallback() {
+        if (!import.meta.env.SSR) {
+            const parser = new DOMParser();
+        }
+    }
+}
+```
+
+```js
+import { LightningElement } from 'lwc';
+
 export default class Foo extends LightningElement {
     renderedCallback() {
         const parser = new DOMParser();
@@ -35,10 +76,27 @@ export default class Foo extends LightningElement {
 import { LightningElement } from 'lwc';
 
 export default class Foo extends LightningElement {
-    renderedCallback() {
-        if (!import.meta.env.SSR) {
-            const parser = new DOMParser();
-        }
+    constructor() {
+        this.handleResize = this.handleResize.bind(this);
+        this.handleClick = this.handleClick.bind(this);
+    }
+
+    connectedCallback() {
+        globalThis.addEventListener?.('resize', this.handleResize);
+        globalThis.document?.addEventListener('click', this.handleClick);
+    }
+
+    disconnectedCallback() {
+        globalThis.removeEventListener?.('resize', this.handleResize);
+        globalThis.document?.removeEventListener('click', this.handleClick);
+    }
+
+    handleResize(event) {
+        /* ... */
+    }
+
+    handleClick(event) {
+        /* ... */
     }
 }
 ```
@@ -49,6 +107,6 @@ The rule takes one option, an object, which has one key `restricted-globals` whi
 are strings which represent the name of the global and the values can be booleans, `true` indicating that the global
 is restricted and `false` to indicate that the global is allowed (useful for overriding an already restricted global).
 
-```js
-{ "restricted-globals": { MyBrowserOnlyGlobal: true, MyAvailableGlobal: false } }
+```json
+{ "restricted-globals": { "MyBrowserOnlyGlobal": true, "MyAvailableGlobal": false } }
 ```
diff --git a/lib/rule-helpers.js b/lib/rule-helpers.js
@@ -128,7 +128,7 @@ function inModuleScope(node, context) {
 
 module.exports.noReferenceDuringSSR = function noReferenceDuringSSR(
     forbiddenGlobalNames,
-    messageId,
+    messageIds,
     context,
 ) {
     const {
@@ -150,15 +150,49 @@ module.exports.noReferenceDuringSSR = function noReferenceDuringSSR(
                 return;
             }
             if (
+                node.parent.type === 'MemberExpression' &&
                 node.object.type === 'Identifier' &&
-                forbiddenGlobalNames.has(node.object.name) &&
+                node.object.name === 'globalThis' &&
+                node.property.type === 'Identifier' &&
+                forbiddenGlobalNames.has(node.property.name) &&
+                node.parent.optional !== true &&
+                isGlobalIdentifier(node.object, context.getScope())
+            ) {
+                // Prevents expressions like:
+                // globalThis.document.addEventListener('click', () => { ... });
+
+                // Allows expressions like:
+                // globalThis.document?.addEventListener('click', () => { ... });
+                context.report({
+                    messageId: messageIds.at(1),
+                    node,
+                    data: {
+                        identifier: node.property.name,
+                        property: node.parent.property.name,
+                    },
+                });
+            } else if (
+                node.parent.type !== 'MemberExpression' &&
+                node.object.type === 'Identifier' &&
+                (forbiddenGlobalNames.has(node.object.name) ||
+                    (node.object.name === 'globalThis' && node.optional !== true)) &&
                 isGlobalIdentifier(node.object, context.getScope())
             ) {
+                // Prevents expressions like:
+                // globalThis.addEventListener('click', () => { ... });
+                // document.addEventListener('click', () => { ... });
+                // document?.addEventListener('click', () => { ... });
+
+                // Allows expressions like:
+                // globalThis?.addEventListener('click', () => { ... });
                 context.report({
-                    messageId,
+                    messageId: messageIds.at(0),
                     node,
                     data: {
-                        identifier: node.object.name,
+                        identifier:
+                            node.object.name === 'globalThis'
+                                ? node.property.name
+                                : node.object.name,
                     },
                 });
             }
@@ -177,8 +211,14 @@ module.exports.noReferenceDuringSSR = function noReferenceDuringSSR(
                 forbiddenGlobalNames.has(node.name) &&
                 isGlobalIdentifier(node, context.getScope())
             ) {
+                // Prevents expressions like:
+                // doSomethingWith(window);
+                // doSomethingWith(document);
+
+                // Allows expressions like:
+                // doSomethingWith(globalThis)
                 context.report({
-                    messageId,
+                    messageId: messageIds.at(0),
                     node,
                     data: {
                         identifier: node.name,

diff --git a/lib/rules/no-restricted-browser-globals-during-ssr.js b/lib/rules/no-restricted-browser-globals-during-ssr.js
@@ -10,6 +10,7 @@ const { noReferenceDuringSSR } = require('../rule-helpers');
 const { docUrl } = require('../util/doc-url');
 
 const forbiddenGlobalNames = new Set(Object.keys(browser));
+forbiddenGlobalNames.delete('globalThis'); // This case we treat separately
 for (const allowedGlobal of Object.keys(nodeBuiltin)) {
     forbiddenGlobalNames.delete(allowedGlobal);
 }
@@ -37,12 +38,14 @@ module.exports = {
         ],
         messages: {
             prohibitedBrowserAPIUsage:
-                'Invalid usage of a browser global API during SSR. Consider moving `{{identifier}}` to the `renderedCallback`.',
+                'Invalid usage of a browser global API during SSR. Consider guarding access to `{{identifier}}`, e.g. via the `import.meta.env.SSR` flag, or optional chaining (`globalThis?.{{identifier}}`).',
+            prohibitedBrowserAPIUsageGlobal:
+                'Invalid usage of a browser global API during SSR. Consider guarding access to `{{identifier}}.{{property}}`, e.g. via the `import.meta.env.SSR` flag, or optional chaining (`globalThis.{{identifier}}?.{{property}}`).',
         },
     },
     create: (context) => {
         const forbiddenGlobals = new Set(forbiddenGlobalNames);
-        const { 'restricted-globals': restrictedGlobals } = context.options[0] || {
+        const { 'restricted-globals': restrictedGlobals } = context.options.at(0) || {
             'restricted-globals': {},
         };
         for (const [global, isRestricted] of Object.entries(restrictedGlobals)) {
@@ -52,6 +55,10 @@ module.exports = {
                 forbiddenGlobals.delete(global);
             }
         }
-        return noReferenceDuringSSR(forbiddenGlobals, 'prohibitedBrowserAPIUsage', context);
+        return noReferenceDuringSSR(
+            forbiddenGlobals,
+            ['prohibitedBrowserAPIUsage', 'prohibitedBrowserAPIUsageGlobal'],
+            context,
+        );
     },
 };
diff --git a/lib/util/scope.js b/lib/util/scope.js
@@ -20,7 +20,7 @@ function isGlobalIdentifier(identifier, scope) {
     // If the reference is not resolved, or if it is resolved in the global scope it means the
     // identifier is global. ESLint automatically add global properties in the global scope
     // depending on the env config.
-    // eg. "browser": true will automatically inject "window" and "setTimeout" in the global scope.
+    // e.g. "browser": true will automatically inject "window" and "setTimeout" in the global scope.
     return ref && (ref.resolved === null || ref.resolved.scope.type === 'global');
 }
 

diff --git a/test/lib/rules/no-restricted-browser-globals-during-ssr.js b/test/lib/rules/no-restricted-browser-globals-during-ssr.js
@@ -202,14 +202,55 @@ tester.run('no-browser-globals-during-ssr', rule, {
               }
           `,
         },
-
         {
             code: `btoa('lwc');`,
         },
         {
             code: `document`,
             options: [{ 'restricted-globals': { document: false } }],
         },
+        {
+            code: `
+            import { LightningElement } from 'lwc';
+
+            export default class Foo extends LightningElement {
+              foo = globalThis.document?.x;
+            }
+          `,
+        },
+        {
+            code: `
+            import { LightningElement } from 'lwc';
+
+            export default class Foo extends LightningElement {
+              constructor() {
+                console.log(globalThis.document?.x);
+              }
+            }
+          `,
+        },
+        {
+            code: `
+              import { LightningElement } from 'lwc';
+
+              export default class Foo extends LightningElement {
+                connectedCallback() {
+                  globalThis.document?.addEventListener('click', () => {});
+                }
+              }
+          `,
+        },
+        {
+            code: `
+                import { LightningElement } from 'lwc';
+
+                export default class Foo extends LightningElement {
+                  connectedCallback() {
+                    doSomethingWith(globalThis);
+                  }
+                }
+            `,
+        },
     ],
     invalid: [
         {
@@ -498,5 +539,105 @@ tester.run('no-browser-globals-during-ssr', rule, {
                 },
             ],
         },
+        {
+            code: `
+            import { LightningElement } from 'lwc';
+
+            export default class Foo extends LightningElement {
+              foo = globalThis.document.x;
+            }
+        `,
+            errors: [
+                {
+                    messageId: 'prohibitedBrowserAPIUsageGlobal',
+                    data: { identifier: 'document', property: 'x' },
+                },
+            ],
+        },
+        {
+            code: `
+            import { LightningElement } from 'lwc';
+
+            export default class Foo extends LightningElement {
+              constructor() {
+                console.log(globalThis.document.x);
+              }
+            }
+        `,
+            errors: [
+                {
+                    messageId: 'prohibitedBrowserAPIUsageGlobal',
+                    data: { identifier: 'document', property: 'x' },
+                },
+            ],
+        },
+        {
+            code: `
+              import { LightningElement } from 'lwc';
+
+              export default class Foo extends LightningElement {
+                connectedCallback() {
+                  document.addEventListener('click', () => {});
+                }
+              }
+          `,
+            errors: [
+                {
+                    messageId: 'prohibitedBrowserAPIUsage',
+                    data: { identifier: 'document' },
+                },
+            ],
+        },
+        {
+            code: `
+              import { LightningElement } from 'lwc';
+
+              export default class Foo extends LightningElement {
+                connectedCallback() {
+                  document?.addEventListener('click', () => {});
+                }
+              }
+          `,
+            errors: [
+                {
+                    messageId: 'prohibitedBrowserAPIUsage',
+                    data: { identifier: 'document' },
+                },
+            ],
+        },
+        {
+            code: `
+              import { LightningElement } from 'lwc';
+
+              export default class Foo extends LightningElement {
+                connectedCallback() {
+                  globalThis.document.addEventListener('click', () => {});
+                }
+              }
+          `,
+            errors: [
+                {
+                    messageId: 'prohibitedBrowserAPIUsageGlobal',
+                    data: { identifier: 'document', property: 'addEventListener' },
+                },
+            ],
+        },
+        {
+            code: `
+              import { LightningElement } from 'lwc';
+
+              export default class Foo extends LightningElement {
+                connectedCallback() {
+                  globalThis.addEventListener('click', () => {});
+                }
+              }
+          `,
+            errors: [
+                {
+                    messageId: 'prohibitedBrowserAPIUsage',
+                    data: { identifier: 'addEventListener' },
+                },
+            ],
+        },
     ],
 });