feat: make module window check more robust #14
Conversation
🦋 Changeset detectedLatest commit: f0474fd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
src/rules/use-client.ts
Outdated
| conditionalExpressionNode.test.left.argument?.type === "Identifier" && | ||
| conditionalExpressionNode.test.left.argument?.name === "window" && |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍
src/rules/use-client.ts
Outdated
| conditionalExpressionNode.alternate === node?.parent) || | ||
| (isNegatedWindowCheck && | ||
| conditionalExpressionNode.consequent === node?.parent); |
There was a problem hiding this comment.
What is conditionalExpressionNode.consequent === node?.parent meant to check? Not sure what this is for
There was a problem hiding this comment.
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
| @@ -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);`, | |||
Co-authored-by: Rogin Farrer <rogin@roginfarrer.com>
| code: `import * as React from 'react'; | ||
| const context = React.createContext() | ||
| export function Foo() { | ||
| return <div />; | ||
| } | ||
| export function useTheme() { | ||
| const context = React.useContext(context); | ||
| React.useEffect(() => { | ||
| window.setTimeout(() => {}); | ||
| }); | ||
| return context; | ||
| }`, |
There was a problem hiding this comment.
I moved these to the invalid section cause honestly I'm not sure why they were valid to being with. From my local testing if I have const context = React.createContext() at the top of a component file and use that component in a server context I get a runtime error. If there's something I'm missing please let me know!
There was a problem hiding this comment.
If we do move forward with it and you feel like this warrants a major LMK 👍
There was a problem hiding this comment.
I think it would probably be best as a major. Happy to leave this in and cut the next release as a major
There was a problem hiding this comment.
@roginfarrer When you say "leave this in" are you saying I should leave my changeset as is? Or update that to a major? Or just remove it all together?
There was a problem hiding this comment.
@roginfarrer just wondering if you have a chance to answer ☝️ and then I can make the changes and get this merged
There was a problem hiding this comment.
Do you know why they pass in both valid and invalid sections? Or did your changes make them invalid?
There was a problem hiding this comment.
Yeah my changes made them invalid. And when I tested the scenario in a small next project, I would get runtime client only errors in the dev server related to the React Context so I think having them be invalid is correct. Please feel free to double check that though!
| //@ts-expect-error | ||
| const { parent } = node; | ||
| if (!parent) { |
There was a problem hiding this comment.
Does this fix the error?
| //@ts-expect-error | |
| const { parent } = node; | |
| if (!parent) { | |
| const parent = node?.parent | |
| if (!parent) { |
There was a problem hiding this comment.
No, the error comes from the parent field apparently not being a part of the BinaryExpression type 🤔
Property 'parent' does not exist on type 'BinaryExpression'.
| code: `import * as React from 'react'; | ||
| const context = React.createContext() | ||
| export function Foo() { | ||
| return <div />; | ||
| } | ||
| export function useTheme() { | ||
| const context = React.useContext(context); | ||
| React.useEffect(() => { | ||
| window.setTimeout(() => {}); | ||
| }); | ||
| return context; | ||
| }`, |
There was a problem hiding this comment.
I think it would probably be best as a major. Happy to leave this in and cut the next release as a major
We started using this rule in one of our repos (and are loving it so far!) and we noticed it was marking files that have patterns like this as needing the client directive
This is fine to execute on the server or the client because we are checking to make sure the window exists before accessing it.
So I took a stab at updating the lint rule to account for this case. This is my first time dabbling in writing a lint rule, so please feel free to make suggestions! Overall we just want to get this case accounted for so we don't run into more issues with files being improperly marked 🙂