Skip to content

feat: make module window check more robust #14

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
+217 −40
Open

feat: make module window check more robust #14

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
de40373
make window check more robust
cmackenthun Mar 5, 2025
558c4bd
add changeset
cmackenthun Mar 5, 2025
98c1fd6
Update .changeset/honest-peas-peel.md
cmackenthun Mar 6, 2025
2996b70
quick cleanup
cmackenthun Mar 6, 2025
9846500
quick cleanup
cmackenthun Mar 6, 2025
fe5dec3
rework
cmackenthun Mar 6, 2025
8a19819
improve changeset
cmackenthun Mar 6, 2025
5ea4aec
change variables to generalize
cmackenthun Mar 6, 2025
943176a
refactor from pr comments
cmackenthun Mar 18, 2025
f0474fd
Update honest-peas-peel.md
roginfarrer Apr 8, 2025
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
5 changes: 5 additions & 0 deletions .changeset/honest-peas-peel.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"eslint-plugin-react-server-components": patch
---

Making window check more robust to not require "use client" when safely accessed behind a `typeof window !== undefined` check
14 changes: 14 additions & 0 deletions src/rules/use-client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,13 @@ describe("use client", () => {
return context;
}`,
},
{
code: `const HREF = typeof window === 'undefined' ? undefined : window.location.href;`
},
{
code: `const HREF = typeof window !== 'undefined' ? window.location.href : '';`
},

],
invalid: [
// DOCUMENT
Expand Down Expand Up @@ -99,6 +106,13 @@ function Bar() {
window.addEventListener('scroll', () => {})
return <div />;
}`,
},
{
code: `const HREF = typeof window === 'undefined' ? window.location.href : window.location.href.slice(0,10);`,
Copy link
Owner

Choose a reason for hiding this comment

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

Good edge case to catch!

errors: [{ messageId: "addUseClientBrowserAPI" }],
output: `'use client';

const HREF = typeof window === 'undefined' ? window.location.href : window.location.href.slice(0,10);`,
},
// OBSERVERS
{
Expand Down
33 changes: 31 additions & 2 deletions src/rules/use-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,11 +208,40 @@ const create = Components.detect(
// @ts-expect-error
const name = node.object.name;
const scopeType = context.getScope().type;
if (

// check if the window usage is behind a typeof window === 'undefined' check
const conditionalExpressionNode = node.parent?.parent;
const isWindowCheck =
conditionalExpressionNode?.type === "ConditionalExpression" &&
conditionalExpressionNode.test?.type === "BinaryExpression" &&
conditionalExpressionNode.test.left?.type === "UnaryExpression" &&
conditionalExpressionNode.test.left.operator === "typeof" &&
conditionalExpressionNode.test.left.argument?.type === "Identifier" &&
conditionalExpressionNode.test.left.argument?.name === "window" &&
Copy link
Owner

Choose a reason for hiding this comment

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

Does this catch other places where we might make this check? Like in an if statement:

if (typeof window !== 'undefined') {
}

We could use Identifier as the node entrypoint, and checking whether it's 'window'. Then navigate the tree to see if it's used safely. Should this also work with other browser globals?

I think for legibility I might recommend extracting this logic into a "checkIfSafeBrowserGlobalUse" function outside of this function (where it takes node as an argument).

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I'm not sure about the if statement or not, but I don't think this would cover that. I was mostly going for the simplest solution to the one edge case we were seeing. I can look into using the Identifier as an entry point but am still pretty new to writing a lint rule, so that might take a bit more effort for me. Would we still need some kind of check under the MemberExpression entrypoint to make sure we skip erroring on these safe checks?

Agreed on the legibility part 👍 I can do that for sure.

Copy link
Author

Choose a reason for hiding this comment

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

Oh nvm, I think you're advocating for removing the MemberExpression entrypoint in favor of a starting at the Identifier level. I'll investigate that a bit 👍

conditionalExpressionNode.test.right?.type === "Literal" &&
conditionalExpressionNode.test.right.value === "undefined";

// checks to see if it's `typeof window !== 'undefined'` or `typeof window === 'undefined'`
const isNegatedWindowCheck =
isWindowCheck &&
conditionalExpressionNode.test?.type === "BinaryExpression" &&
conditionalExpressionNode.test.operator === "!==";

// checks to see if window is being accessed safely behind a window check
const isSafelyBehindWindowCheck =
(isWindowCheck &&
!isNegatedWindowCheck &&
conditionalExpressionNode.alternate === node?.parent) ||
(isNegatedWindowCheck &&
conditionalExpressionNode.consequent === node?.parent);
Copy link
Owner

Choose a reason for hiding this comment

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

What is conditionalExpressionNode.consequent === node?.parent meant to check? Not sure what this is for

Copy link
Author

Choose a reason for hiding this comment

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

This is basically to cover either typeof window === undefined ? <server code> : <client code> OR typeof window !== undefined ? <client code> : <server code>. Basically just checking if the alternate or the consequent is the client code based on the isNegatedWindowCheck value


if (
undeclaredReferences.has(name) &&
browserOnlyGlobals.has(name) &&
(scopeType === "module" || !!util.getParentComponent(node))
(scopeType === "module" || !!util.getParentComponent(node)) &&
!isSafelyBehindWindowCheck
) {
// console.log(name, node.object)
instances.push(name);
reportMissingDirective("addUseClientBrowserAPI", node.object);
}
Expand Down