@W-17070457 Allow multi conditional !import.meta.env.SSR statements #171

a-chabot · 2024-11-13T22:44:16Z

@W-17070457 Allow multi conditional !import.meta.env.SSR statements

@lwc/lwc/ssr/no-unsupported-properties did not recognize when there is a multi conditional check with !import.meta.env.SSR.

Example: These is the scenario we are trying to fix:

if (!import.meta.env.SSR && randomOtherCheck) {
  this.dispatchEvent(someEvent);
} 

if (randomOtherCheck && !import.meta.env.SSR) {
  this.dispatchEvent(someEvent);
} 

if (!a && b && !c && d && !import.meta.env.SSR) {
  this.dispatchEvent(someEvent);
} 

let hello = (!import.meta.env.SSR && a) ? yes : no

if (!import.meta.env.SSR) {
  this.dispatchEvent(someEvent);
}

nolanlawson

Initial round of feedback. Looking good overall!

nolanlawson · 2024-11-19T00:49:58Z

lib/util/ssr.js

@@ -6,13 +6,45 @@
 */
 'use strict';

+let ssrEscapeFound = false;


Having this let outside of isSSREscape makes it a global. This is a code smell to me – I'd recommend moving both ssrEscapeFound and checkConditionalStatements into isSSREscape instead.

nolanlawson · 2024-11-19T00:54:16Z

lib/util/ssr.js

-    );
+    if (node.type === 'IfStatement' || node.type === 'ConditionalExpression') {
+        ssrEscapeFound = false;
+        if (checkConditionalStatements(node.test) || isWindowOrDocumentCheck(node.test)) {


Since typeof window !== 'undefined' is basically the same as !import.meta.env.SSR, I think we should handle the recursive case here too:

if (randomOtherCheck && typeof window !== 'undefined') {

nolanlawson · 2024-11-19T00:55:21Z

lib/util/ssr.js

+    // Recursive Case: If the node is a logical expression, check its left and right parts.
+    if (node.type === 'LogicalExpression' || node.type === 'BinaryExpression') {
+        checkConditionalStatements(node.left);
+        checkConditionalStatements(node.right);


Rather than checking the ssrEscapeFound global, you could have checkConditionalStatements return a boolean, and then do a |= here.

nolanlawson · 2024-11-19T00:55:38Z

lib/util/ssr.js

+        checkConditionalStatements(node.right);
+    }
+
+    // Base Case: If the node is an Identifier or MemberExpression, don't need to continue.


If the node is an Identifier or MemberExpression, then there is no way it can be !import.meta.env.SSR. This is a base case to break out of the recursion. Since it's not the base case we care about (UnaryExpression), there is nothing more to check.

This would be a helpful comment! 🙂

Actually, this if seems redundant to me since we fall through to return false on line 47 anyway. I think we can remove it entirely.

lib/util/ssr.js

nolanlawson · 2024-11-19T00:58:30Z

test/lib/rules/no-unsupported-ssr-properties.js

@@ -351,7 +401,6 @@ testRule('no-unsupported-ssr-properties', {
        {
            code: `
                import { LightningElement } from 'lwc';
-
                export default class Foo extends LightningElement {
                  connectedCallback() {
                    this.childNodes.item(0).textContent = 'foo';


Having some invalid cases tested here would be helpful to check that the PR is working as expected. For example:

if (a && b && c && d) { this.template.querySelector('span'); }

nolanlawson · 2024-11-19T01:00:45Z

BTW one slightly broken thing I noticed is that isWindowOrDocumentCheck doesn't bother to check the right-hand-side:

eslint-plugin-lwc/lib/util/ssr.js

Lines 54 to 62 in c5d0cbb

    
           function isWindowOrDocumentCheck(node) { 
        
               return ( 
        
                   node.type === 'BinaryExpression' && 
        
                   node.left.type === 'UnaryExpression' && 
        
                   node.left.operator === 'typeof' && 
        
                   node.left.argument.type === 'Identifier' && 
        
                   (node.left.argument.name === 'document' || node.left.argument.name === 'window') 
        
               ); 
        
           }

This means that it treats typeof document === 'undefined' the same as typeof document === 'object'. This doesn't need to be fixed in this PR, but it is odd since they mean completely different things.

lib/util/ssr.js

Co-authored-by: Nolan Lawson <[email protected]>

nolanlawson

LGTM overall, some minor suggestions for cleanup.

lib/util/ssr.js

nolanlawson · 2024-12-03T23:04:19Z

test/lib/rules/no-unsupported-ssr-properties.js

+        {
+            code: `
+              import { LightningElement } from 'lwc';
+
+              export default class Foo extends LightningElement {
+                connectedCallback() {
+                  return isCSR ? this.template.querySelector('button') : null;
+                }
+              }
+          `,
+        },


This doesn't look correct at all. This test is only passing because it's using this.template.querySelector rather than this.querySelector. We should open a separate issue to handle this.template.querySelector the same as this.querySelector.

Suggested change

{

code: `

import { LightningElement } from 'lwc';

export default class Foo extends LightningElement {

connectedCallback() {

return isCSR ? this.template.querySelector('button') : null;

}

}

`,

},

{

code: `

import { LightningElement } from 'lwc';

export default class Foo extends LightningElement {

connectedCallback() {

return isCSR ? this.template.querySelector('button') : null;

}

}

`,

},

[SSR] this.querySelector* detected, but not this.template.querySelector* #174

I've updated the test to be this:

import { LightningElement } from 'lwc'; export default class Foo extends LightningElement { connectedCallback() { return !import.meta.env.SSR ? this.querySelector('button') : null; } }

Seems to pass!

Not sure an issue was opened but we have plans to migrate all isCSR into !import.meta.env.SSR because they were not getting detected by the linter.

nolanlawson

LGTM! Minor nitpicks, but

lib/util/ssr.js

nolanlawson · 2024-12-04T18:13:07Z

lib/util/ssr.js

+    // Recursive Case: If the node is a logical expression, check its left and right parts.
+    if (node.type === 'LogicalExpression' && node.operator === '&&') {
+        const rightNodeConditional = checkConditionalStatements(node.right);
+        return !!rightNodeConditional || checkConditionalStatements(node.left);


If we want to be consistent about returning booleans (!!) then we'll also need to fix isMetaEnvCheck since it can return undefined if it returns node.type.

Or we can just remove the !! because it's probably not important to return a boolean versus truthy/falsy. 🙂

Co-authored-by: Nolan Lawson <[email protected]>

a-chabot added 7 commits November 13, 2024 17:16

checkConditionalStatement

cda4062

test attempt 1

894a09d

formatter

ed5fd4b

fixed returns

d8e13b6

format

a460184

variable reset

8405da2

get rid of original

5bb6865

nolanlawson requested changes Nov 19, 2024

View reviewed changes

nolanlawson reviewed Nov 19, 2024

View reviewed changes

lib/util/ssr.js Outdated Show resolved Hide resolved

a-chabot added 6 commits November 19, 2024 10:34

operator and invalid test

9458166

remove ssrEscapeFound

61da9ee

ignore or symbol

a6051fe

spaces

5bcd21c

spaces

51fbf1c

spaces

db05587

nolanlawson reviewed Nov 22, 2024

View reviewed changes

lib/util/ssr.js Outdated Show resolved Hide resolved

a-chabot and others added 4 commits November 27, 2024 10:01

Update lib/util/ssr.js

54c8123

Co-authored-by: Nolan Lawson <[email protected]>

remove extra base case bc default is false

8d09a50

binary expressions

9a45552

expand conditional statements

8dec5dc

a-chabot requested a review from nolanlawson November 27, 2024 15:45

linter

a414851

nolanlawson requested changes Dec 3, 2024

View reviewed changes

a-chabot added 2 commits December 4, 2024 10:19

nolan review notes

2f4505d

simplify logical expressions

d893920

nolanlawson approved these changes Dec 4, 2024

View reviewed changes

a-chabot and others added 3 commits December 4, 2024 15:18

Update lib/util/ssr.js with !

759bd26

Co-authored-by: Nolan Lawson <[email protected]>

Update lib/util/ssr.js with &&

b071787

Co-authored-by: Nolan Lawson <[email protected]>

remove !!

26b8e23

nolanlawson merged commit c57c51f into salesforce:master Dec 4, 2024
6 checks passed

a-chabot deleted the a-chabot/multiconditions branch December 4, 2024 21:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

@W-17070457 Allow multi conditional !import.meta.env.SSR statements #171

@W-17070457 Allow multi conditional !import.meta.env.SSR statements #171

a-chabot commented Nov 13, 2024 •

edited

Loading

nolanlawson left a comment

nolanlawson Nov 19, 2024

nolanlawson Nov 19, 2024

nolanlawson Nov 19, 2024

nolanlawson Nov 19, 2024

a-chabot Nov 19, 2024 •

edited

Loading

nolanlawson Nov 22, 2024

nolanlawson Nov 22, 2024

nolanlawson Nov 19, 2024

nolanlawson commented Nov 19, 2024

nolanlawson left a comment

nolanlawson Dec 3, 2024

nolanlawson Dec 3, 2024

a-chabot Dec 4, 2024

a-chabot Dec 4, 2024

nolanlawson left a comment

nolanlawson Dec 4, 2024

@W-17070457 Allow multi conditional !import.meta.env.SSR statements #171

@W-17070457 Allow multi conditional !import.meta.env.SSR statements #171

Conversation

a-chabot commented Nov 13, 2024 • edited Loading

nolanlawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-chabot Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanlawson commented Nov 19, 2024

nolanlawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanlawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-chabot commented Nov 13, 2024 •

edited

Loading

a-chabot Nov 19, 2024 •

edited

Loading