feat: add eslint rules for SSR #107

abdulsattar · 2022-12-19T10:08:40Z

@divmain is the main author of this commit. I'm moving them from his repo this.

This PR introduces five new rules that check that browser APIs aren't used anywhere where SSR is done.

It will fail only if SSR-unfriendly code is found in specific methods. For example, you can use this.template.querySelector in renderedCallback but not in connectedCallback.
This is also true if your lifecycle method invokes another method on the LWC. For example, if connectedCallback calls this.foo() and foo tries to access document, the linter will complain.
This is also true for functions defined in module scope. For example, connectedCallback might call doSomething(), which is defined in the same module like so function doSomething() { document.write("<div/>") }. In this case, an error will be surfaced.
The linter cannot follow invocation patterns across module boundaries. For example, if doSomething is defined in a different module than your LWC, you're on your own and the linter can't help you.
Most browser APIs are locked down. You can't use this.template in SSR. You can't touch most browser global variables that aren't also available in Node.
Any if statements that have if(document !== undefined) are ignored. We don't check the else blocks as well.

.eslintrc

docs/rules/no-document-during-ssr.md

docs/rules/no-querying-slots-during-ssr.md

docs/rules/no-restricted-browser-globals-during-ssr.md

docs/rules/no-this-template-during-ssr.md

docs/rules/no-window-during-ssr.md

lib/rules/no-restricted-browser-globals-during-ssr.js

docs/rules/no-restricted-browser-globals-during-ssr.md

docs/rules/no-this-property-during-ssr.md

divmain

This looks great to me!

lib/analyze-component.js

divmain · 2023-02-01T05:25:07Z

lib/rules/no-restricted-browser-globals-during-ssr.js

+    'captureEvents',
+    'chrome',
+    'clientInformation',
+    // 'close',


We chatted about this on Slack, but also leaving this here for other reviewers & for posterity:

In the future, we can use ESlint's scope API to determine whether a variable is locally scoped or a global. When that is in place, we can uncomment these global variables with common names.

In the future, we can use ESlint's scope API to determine whether a variable is locally scoped or a global.

IMO, it's something we should have from day 1. If an ESLint rule reports noise, developers will certainly disable this rule globally and never enable it again.

I am ok with merging this PR to get the ball rolling, but I don't think those rules should be used until we resolve this scoping issue.

pmdartus · 2023-02-01T08:33:22Z

lib/rules/no-restricted-browser-globals-during-ssr.js

+        schema: [],
+        messages: {
+            prohibitedBrowserAPIUsage:
+                '`{{identifier}}`, like most browser APIs, is not accessible during SSR.',


Suggested change

'`{{identifier}}`, like most browser APIs, is not accessible during SSR.',

'Invalid usage of a browser global API during SSR. Consider moving `{{identifier}}` to the `renderedCallback` ',

pmdartus · 2023-02-01T08:36:31Z

lib/rules/no-restricted-browser-globals-during-ssr.js

+const { noReferenceDuringSSR } = require('../rule-helpers');
+const { docUrl } = require('../util/doc-url');
+
+const prohibitedGlobalVariables = new Set([


The list of known browser APIs is consistently evolving. We should avoid having to maintain this list.
ESLint is using internal the globals npm package to determine which global is available per environment. I think we should do the same.

pmdartus · 2023-02-01T08:40:21Z

lib/rules/no-restricted-browser-globals-during-ssr.js

+    'captureEvents',
+    'chrome',
+    'clientInformation',
+    // 'close',


In the future, we can use ESlint's scope API to determine whether a variable is locally scoped or a global.

IMO, it's something we should have from day 1. If an ESLint rule reports noise, developers will certainly disable this rule globally and never enable it again.

I am ok with merging this PR to get the ball rolling, but I don't think those rules should be used until we resolve this scoping issue.

lib/rules/no-restricted-browser-globals-during-ssr.js

lib/walk.js

pmdartus · 2023-02-01T09:00:08Z

test/lib/rules/no-this-property-during-ssr.js

+
+const tester = new RuleTester(ESLINT_TEST_CONFIG);
+
+const disallowedProperties = [


Ditto. Let's avoid having to maintain multiple disallow lists.

pmdartus · 2023-02-01T09:01:40Z

test/lib/rules/no-this-property-during-ssr.js

+            errors: [
+                {
+                    message:
+                        'You should not use `this.template.querySelector` in methods that will execute during SSR. Use `lwc:ref` instead.',


This error message is misleading. Even with refs, you can't access the rendered element in the connectedCallback (on the client nor the server).

pmdartus · 2023-02-01T09:05:14Z

test/lib/rules/no-this-property-during-ssr.js

+            errors: [
+                {
+                    message:
+                        'You should not access any DOM properties on `this` in methods that will execute during SSR.',


You should not access any DOM properties on this in methods that will execute during SSR.

Is this statement really true? As far as I know, we do implement at least this.classList, this.getAttribute, and this.setAttribute. Does it mean that we should retire those APIs?

Reading is fine. Writing is not fine and causes hydration errors. An improvement would be to restrict the developer only from using APIs that change the DOM. Would you be okay with this going into a follow-up PR, @pmdartus?

Sure. I don't see any issue with doing this work in a follow-up PR as long as we do it prior to socializing those new rules.

pmdartus · 2023-02-01T09:07:02Z

lib/rules/no-this-property-during-ssr.js

+const { noPropertyAccessDuringSSR } = require('../rule-helpers');
+const { docUrl } = require('../util/doc-url');
+
+const disallowedProperties = [


Only a subset of those APIs is implemented by LightningElement today. Could we be more selective?

pmdartus · 2023-02-01T09:12:34Z

docs/rules/no-this-property-during-ssr.md

@@ -0,0 +1,48 @@
+# Disallow access of properties on this during SSR (`lwc/no-this-property-during-ssr`)


I would like to better understand the motivation behind this rule. It appears to me that we are bundling multiple potential SSR errors in a single rule:

Access to certain DOM APIs that aren't implemented by the LWC engine server: eg. dispatchEvent

Access to rendered content prior to the component being rendered this[.template].querySelector

Access to the parent node via the template this.template.host

Access to DOM APIs poly filled on the LWC engine server: eg. this.getAttribute, this.setAttribute, this.classList

Could we tease those use cases apart and evaluate the usefulness of each of them individually?

lib/rules/no-restricted-browser-globals-during-ssr.js

pmdartus · 2023-02-17T13:26:39Z

lib/util/ssr.js

+        node.test.left.type === 'UnaryExpression' &&
+        node.test.left.operator === 'typeof' &&
+        node.test.left.argument.type === 'Identifier' &&
+        (node.test.left.argument.name === 'document' || node.test.left.argument.name === 'window')


This is also something we should parameterize. I would expect LWR to come with a more elegant option than type 'window' checks.

I think if we land imports.meta.env, we can change this to check for that.

Sound good to me. Could you add a comment here, to make sure we track this in the future?

README.md

pmdartus

There is one bug I would recommend addressing, and a couple of comments. Otherwise looks good.

pmdartus · 2023-02-20T14:54:00Z

lib/rules/no-restricted-browser-globals-during-ssr.js

+        const { 'restricted-globals': restrictedGlobals } = context.options[0] || {
+            'restricted-globals': {},
+        };
+        for (const [global, isRestricted] of Object.entries(restrictedGlobals)) {


Since rules can be applied on a per-file basis, we shouldn't mutate the forbiddenGlobalNames singleton object. The forbiddenGlobalNames should be copied first before applying the configuration.

We are not modifying the singleton. In line 44, we're cloning it and modifying the clone.

You are right. I haven't read the code carefully enough.

pmdartus · 2023-02-20T14:55:05Z

lib/util/ssr.js

+        node.test.left.type === 'UnaryExpression' &&
+        node.test.left.operator === 'typeof' &&
+        node.test.left.argument.type === 'Identifier' &&
+        (node.test.left.argument.name === 'document' || node.test.left.argument.name === 'window')


Sound good to me. Could you add a comment here, to make sure we track this in the future?

docs/rules/no-restricted-browser-globals-during-ssr.md

abdulsattar added 4 commits December 19, 2022 15:37

feat: transfer rules from eslint-plugin-ssr to this repo

f22eb7c

fix: remove unused code and add more tests

b885d79

chore: fix indents

635eee9

doc: add documentation

35ff8ac

abdulsattar marked this pull request as ready for review December 20, 2022 09:42

abdulsattar marked this pull request as draft December 20, 2022 09:46

feat: ignore blocks of if(document !== undefined).

f724eea

abdulsattar force-pushed the abdulsattar/ssr branch from 5c077e6 to f724eea Compare January 20, 2023 10:40

abdulsattar requested review from pmdartus and divmain January 20, 2023 10:44

abdulsattar marked this pull request as ready for review January 20, 2023 10:46

pmdartus requested changes Jan 24, 2023

View reviewed changes

abdulsattar added 4 commits January 30, 2023 10:43

chore: remove recommended from docs

769e453

chore: remove no-window-during-ssr and no-document-during-ssr

4d111ba

chore: remove references from documentation

a9cfee4

chore: allow common names for reference during ssr

01929b7

abdulsattar commented Jan 30, 2023

View reviewed changes

lib/rules/no-restricted-browser-globals-during-ssr.js Outdated Show resolved Hide resolved

abdulsattar added 7 commits January 31, 2023 17:01

chore: merge otehr rules

418acbd

fix: fix tests

b5952fd

fix: add more tests

6ab19aa

chore: make error messages better

90952e8

fix: report better error message for querySelector

aa5724e

fix: handle as well

821b24e

chore: downgrade eslint parser version

e296a7b

kevinv11n reviewed Jan 31, 2023

View reviewed changes

docs/rules/no-restricted-browser-globals-during-ssr.md Outdated Show resolved Hide resolved

docs/rules/no-restricted-browser-globals-during-ssr.md Outdated Show resolved Hide resolved

docs/rules/no-this-property-during-ssr.md Outdated Show resolved Hide resolved

fix: typos

8ae21fe

divmain approved these changes Feb 1, 2023

View reviewed changes

chore: comments

41e74ad

pmdartus reviewed Feb 1, 2023

View reviewed changes

fix: check for scope to ascertain if a variable is indeed a global

a0bc849

abdulsattar added 8 commits February 3, 2023 10:48

Merge branch 'master' into abdulsattar/ssr

0fbb9e1

chore: remove unnecessary components in test

dbac352

doc: switch to typeof document !== 'undefined''

632608c

chore: use existing helper for determining if variable is global

5b7cd64

chore: use different error message

5e0773e

chore: whitelist a few globals

e6d4415

fix: rename rule to avoid only unsupported SSR methods

f4c3557

Merge branch 'master' into abdulsattar/ssr

b551ef7

abdulsattar requested review from pmdartus and divmain February 17, 2023 12:32

pmdartus reviewed Feb 17, 2023

View reviewed changes

abdulsattar added 3 commits February 20, 2023 12:51

add option for restricted globals

e1c5b65

fix: doc

61351cf

doc: fix unsupported ssr docs

9cb8399

abdulsattar requested a review from pmdartus February 20, 2023 07:28

pmdartus approved these changes Feb 20, 2023

View reviewed changes

doc: add doc, comments

9733a16

abdulsattar merged commit 286da2d into master Feb 20, 2023

abdulsattar deleted the abdulsattar/ssr branch February 20, 2023 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add eslint rules for SSR #107

feat: add eslint rules for SSR #107

abdulsattar commented Dec 19, 2022 •

edited

Loading

divmain left a comment

divmain Feb 1, 2023

pmdartus Feb 1, 2023

pmdartus Feb 1, 2023

pmdartus Feb 1, 2023

pmdartus Feb 1, 2023

pmdartus Feb 1, 2023

pmdartus Feb 1, 2023

pmdartus Feb 1, 2023

divmain Feb 1, 2023

pmdartus Feb 2, 2023

pmdartus Feb 1, 2023

pmdartus Feb 1, 2023

pmdartus Feb 17, 2023

abdulsattar Feb 20, 2023

pmdartus Feb 20, 2023

pmdartus left a comment

pmdartus Feb 20, 2023

abdulsattar Feb 20, 2023

pmdartus Feb 20, 2023

pmdartus Feb 20, 2023

	'`{{identifier}}`, like most browser APIs, is not accessible during SSR.',
	'Invalid usage of a browser global API during SSR. Consider moving `{{identifier}}` to the `renderedCallback` ',


		const tester = new RuleTester(ESLINT_TEST_CONFIG);

		const disallowedProperties = [

		@@ -0,0 +1,48 @@
		# Disallow access of properties on this during SSR (`lwc/no-this-property-during-ssr`)

feat: add eslint rules for SSR #107

feat: add eslint rules for SSR #107

Conversation

abdulsattar commented Dec 19, 2022 • edited Loading

divmain 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

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

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

Choose a reason for hiding this comment

pmdartus 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

abdulsattar commented Dec 19, 2022 •

edited

Loading