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.

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

Rule proposal: ssr-friendly #2057

Open
alexreardon opened this issue Nov 26, 2018 · 21 comments · May be fixed by #2692
Open

Rule proposal: ssr-friendly #2057

alexreardon opened this issue Nov 26, 2018 · 21 comments · May be fixed by #2692

Comments

@alexreardon
Copy link

alexreardon commented Nov 26, 2018

In order to be SSR compliant, you can only access browser globals such as window, document and so on, inside of componentDidMount, componentDidUpdate and componentWillUnmount

screen shot 2018-11-26 at 11 05 04 am

A rule could be created to avoid the use of unguarded browser globals in other lifecycle methods.

Example of a guard:

if (typeof window !== 'undefined') {
 // I can use browser globals in here
}

Use of browser globals are allowed when:

  • used in a guard
  • used in componentDidMount, componentDidUpdate and componentWillUnmount
  • used in useEffect or useLayoutEffect
  • used in an event handler
@ljharb
Copy link
Member

ljharb commented Nov 26, 2018

This sounds like an excellent rule, and I'm very much in favor of it. A PR is appreciated!

(Note, however, that it's also acceptable to access these things in event handlers - any component method or function that is only referenced by passing it in as a prop to an element, and never directly invoked)

@alexreardon
Copy link
Author

@ljharb I updated the description

@alexreardon
Copy link
Author

alexreardon commented Dec 2, 2018

I have not had experience writing eslint plugins before. I see the first step as fairly straight forward: checking if browser globals are called inside of componentDidMount, componentDidUpdate or componentWillUnmount.

I am not sure how to check if this is done inside of a guard such as

if (typeof window !== 'undefined') {
 // I can use browser globals in here
}

@ljharb you have any thoughts on how I could check to see if the usage of the code is guarded?

@ljharb
Copy link
Member

ljharb commented Dec 3, 2018

Even if guarded, if initial render behavior differs between client and server, that’s a bug - so there shouldn’t be any use outside of those methods, even guarded.

@alexreardon
Copy link
Author

Sometimes there might be - such as when a component mounts into a portal.

@alexreardon
Copy link
Author

So on the server it might just return null, whereas in the browser it might return ReactDOM.createPortal(children, el). I have seen this pattern around

@alexreardon
Copy link
Author

But I could buy that these are edge cases and a consumer could add a //eslint-ignore-next-line react/ssr-friendly

@ljharb
Copy link
Member

ljharb commented Dec 3, 2018

That would cause a render mismatch, i believe - if you want to conditionally render into a portal, you’d need to do that using state set in componentDidMount.

@alexreardon
Copy link
Author

Sounds good to me

@sidharthachatterjee
Copy link

Watching this closely as this would be super useful for Gatsby

@alexreardon
Copy link
Author

I hope to take more of a look at this soon!

@golopot
Copy link
Contributor

golopot commented May 25, 2019

One problem: this rule can only detect dom globals if the it is used in the currently linted file. That is, if you import a apiSomethingService, this plugin cannot inspect the import source file to find out whether it is using dom globals.

@ljharb
Copy link
Member

ljharb commented May 25, 2019

Sure, this only would ensure that your own components are authored correctly, much like any linting rule :-)

@mflores-verys
Copy link

@alexreardon I'm very interested in this rule – let me know if you could use an assist in any way! Caveat: we both have the same experience level in writing ESLint plugins 😄

@correttojs
Copy link

I'd also be interested in such rule, as mentioned above it could only detect dom globals within the currently linted file. Despite that, in FC case we can always rename an external service to a custom hook notation, thus enforcing the access within the useEffect. Detect event handlers, promises, async may also be difficult

@Jpunt
Copy link

Jpunt commented Apr 16, 2020

@alexreardon (or anyone), did you find the time to implement this? 😁

@jzabala jzabala linked a pull request Jun 30, 2020 that will close this issue
4 tasks
@kopiro
Copy link

kopiro commented Oct 6, 2020

Hey all, what do you think of this implementation?
https://github.com/kopiro/eslint-plugin-ssr-friendly

@kopiro
Copy link

kopiro commented Oct 7, 2020

@alexreardon

@osdiab
Copy link

osdiab commented Oct 24, 2020

As a workaround I'm also going to consider just disallowing window and global altogether in lint, exempting a file that exports helpers like getWindow() which returns either window or undefined (same for other browser globals like document), and then letting TypeScript enforce that you handle undefined properly. Just throwing the idea out :)

EDIT, here's my implementation. eslintrc.js:

module.exports = {
  // ...
  rules: {
    "no-restricted-globals": [
      "error",
      {
        name: "window",
        message: "Use getBrowserGlobals() function to access window",
      },
      {
        name: "document",
        message: "Use getBrowserGlobals() function to access document",
      },
      // add more unsafe browser globals if necessary
    ],
  }
}

and globals.ts:

/**
 * Gets browser builtin globals in an server-side-rendering-safe way
 *
 * @returns The globals if in a browser environment, or undefined if in a
 * SSR environment
 */
export function getBrowserGlobals() {
  // eslint-disable-next-line no-restricted-globals
  return typeof window === "undefined" ? undefined : { window, document };
}

Finally a use case from a function that scrolls to an element:

getBrowserGlobals()?.document.querySelector(hash)?.scrollIntoView();

nice thing about this is that it doesn't require anyone to learn obscure/platform-specific stuff, like "in React it's safe to use window or document in a useEffect() hook but not in a useMemo() callback" and "you must check for window via typeof window === "undefined" or else your program will crash"; and together with TypeScript it enforces that you properly check for presence/handle it if it's missing. Will report if my team runs into problems with this approach.

@pelhage
Copy link

pelhage commented Mar 5, 2021

Hey @osdiab How did your approach work out? I'm thinking of going into a similar direction for my team as well.

Ideally engineers can safely write SSR compatible code without having to think about all the various edge cases and SSR vs CSR

@osdiab
Copy link

osdiab commented Mar 6, 2021

@pelhage it worked fine for us! No issues throughout my team so far.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

10 participants