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.

Sign up for GitHub

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

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

Merged
merged 23 commits into from
Dec 4, 2024
Merged

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

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
cda4062
checkConditionalStatement
a-chabot Nov 13, 2024
894a09d
test attempt 1
a-chabot Nov 14, 2024
ed5fd4b
formatter
a-chabot Nov 14, 2024
d8e13b6
fixed returns
a-chabot Nov 18, 2024
a460184
format
a-chabot Nov 18, 2024
8405da2
variable reset
a-chabot Nov 18, 2024
5bb6865
get rid of original
a-chabot Nov 18, 2024
9458166
operator and invalid test
a-chabot Nov 19, 2024
61da9ee
remove ssrEscapeFound
a-chabot Nov 21, 2024
a6051fe
ignore or symbol
a-chabot Nov 21, 2024
5bcd21c
spaces
a-chabot Nov 22, 2024
51fbf1c
spaces
a-chabot Nov 22, 2024
db05587
spaces
a-chabot Nov 22, 2024
54c8123
Update lib/util/ssr.js
a-chabot Nov 27, 2024
8d09a50
remove extra base case bc default is false
a-chabot Nov 27, 2024
9a45552
binary expressions
a-chabot Nov 27, 2024
8dec5dc
expand conditional statements
a-chabot Nov 27, 2024
a414851
linter
a-chabot Dec 2, 2024
2f4505d
nolan review notes
a-chabot Dec 4, 2024
d893920
simplify logical expressions
a-chabot Dec 4, 2024
759bd26
Update lib/util/ssr.js with !
a-chabot Dec 4, 2024
b071787
Update lib/util/ssr.js with &&
a-chabot Dec 4, 2024
26b8e23
remove !!
a-chabot Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 36 additions & 4 deletions lib/util/ssr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,44 @@
'use strict';

module.exports.isSSREscape = function isSSREscape(node) {
return (
(node.type === 'IfStatement' || node.type === 'ConditionalExpression') &&
(isMetaEnvCheck(node.test) || isWindowOrDocumentCheck(node.test))
);
if (node.type === 'IfStatement' || node.type === 'ConditionalExpression') {
if (checkConditionalStatements(node.test)) {
return true;
}
}
a-chabot marked this conversation as resolved.
Show resolved Hide resolved

return false;
};

function checkConditionalStatements(test) {
let node = test;

// Base Case: If the node is UnaryExpression, call isMetaEnvCheck().
a-chabot marked this conversation as resolved.
Show resolved Hide resolved
if (node.type === 'UnaryExpression' && node.operator === '!') {
return isMetaEnvCheck(node);
}

// Base Case: If the node is a BinaryExpresion, call isWindowOrDocumentCheck().
a-chabot marked this conversation as resolved.
Show resolved Hide resolved
if (node.type === 'BinaryExpression' && node.operator === '!==') {
return isWindowOrDocumentCheck(node);
}

// Recursive Case: If the node is a logical expression, check its left and right parts.
a-chabot marked this conversation as resolved.
Show resolved Hide resolved
if (
node.type === 'LogicalExpression' ||
(node.type === 'BinaryExpression' && node.operator != '!===')
) {
if (node.operator === '||' || node.operator === '>' || node.operator === '>>') {
return false;
}
a-chabot marked this conversation as resolved.
Show resolved Hide resolved

const rightNodeConditional = checkConditionalStatements(node.right);
return !!rightNodeConditional || checkConditionalStatements(node.left);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🙂

}

return false;
}

function isMetaEnvCheck(test) {
let node = test;
if (!(node.type === 'UnaryExpression' && node.operator === '!')) return false;
Expand Down
179 changes: 179 additions & 0 deletions test/lib/rules/no-unsupported-ssr-properties.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,95 @@ testRule('no-unsupported-ssr-properties', {
}
`,
},
{
code: `
import { LightningElement } from 'lwc';

export default class Foo extends LightningElement {
connectedCallback() {
if (!import.meta.env.SSR && !randomOtherCheck) {
this.querySelector('span').getAttribute('role');
}
}
}
`,
},
{
code: `
import { LightningElement } from 'lwc';

export default class Foo extends LightningElement {
connectedCallback() {
if (!import.meta.env.SSR && randomOtherCheck) {
this.querySelector('span').getAttribute('role');
}
}
}
`,
},
{
code: `
import { LightningElement } from 'lwc';

export default class Foo extends LightningElement {
connectedCallback() {
if (randomOtherCheck && !import.meta.env.SSR) {
this.querySelector('span').getAttribute('role');
}
}
}
`,
},
{
code: `
import { LightningElement } from 'lwc';

export default class Foo extends LightningElement {
connectedCallback() {
if (!a && b && !c && d && !import.meta.env.SSR) {
this.querySelector('span').getAttribute('role');
}
}
}
`,
},
{
code: `
import { LightningElement } from 'lwc';

export default class Foo extends LightningElement {
connectedCallback() {
if (a && (b && !import.meta.env.SSR)) {
this.querySelector('span').getAttribute('role');
}
}
}
`,
},
{
code: `
import { LightningElement } from 'lwc';

export default class Foo extends LightningElement {
connectedCallback() {
return isCSR ? this.template.querySelector('button') : null;
}
}
`,
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}
}
`,
},

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
code: `
import { LightningElement } from 'lwc';

export default class Foo extends LightningElement {
connectedCallback() {
if (randomOtherCheck && typeof window !== 'undefined') {
this.querySelector('span').getAttribute('role');
}
}
}
`,
},
],
invalid: [
{
Expand Down Expand Up @@ -364,5 +453,95 @@ testRule('no-unsupported-ssr-properties', {
},
],
},
{
code: `
import { LightningElement } from 'lwc';

export default class Foo extends LightningElement {
connectedCallback() {
if (!a && b && !c && d) {
this.querySelector('span').getAttribute('role');
}
}
}
`,
errors: [
{
messageId: 'propertyAccessFound',
},
],
},
{
code: `
import { LightningElement } from 'lwc';

export default class Foo extends LightningElement {
connectedCallback() {
if (a && (b || !import.meta.env.SSR)) {
this.querySelector('span').getAttribute('role');
}
}
}
`,
errors: [
{
messageId: 'propertyAccessFound',
},
],
},
{
code: `
import { LightningElement } from 'lwc';

export default class Foo extends LightningElement {
connectedCallback() {
if (a || (b || !import.meta.env.SSR)) {
this.querySelector('span').getAttribute('role');
}
}
}
`,
errors: [
{
messageId: 'propertyAccessFound',
},
],
},
{
code: `
import { LightningElement } from 'lwc';

export default class Foo extends LightningElement {
connectedCallback() {
if (a || (b && !import.meta.env.SSR)) {
this.querySelector('span').getAttribute('role');
}
}
}
`,
errors: [
{
messageId: 'propertyAccessFound',
},
],
},
{
code: `
import { LightningElement } from 'lwc';

export default class Foo extends LightningElement {
connectedCallback() {
if (randomOtherCheck && typeof window == 'undefined') {
this.querySelector('span').getAttribute('role');
}
}
}
`,
errors: [
{
messageId: 'propertyAccessFound',
},
],
},
],
});