Request: Include SSR-related rules

[lesliecdubs]

Would you consider adding SSR-related rules to the recommended React config in the eslint ruleset? This would help catch SSR incompatibility during development instead of requiring more expensive remediation later.

[JoseInTheArena]

Some more context here: https://github.com/github/core-ux/discussions/102#discussioncomment-5512115

[erinnachen]

It looks like the ssr-friendly plugin was added to primer/react in primer/react#3164. @lesliecdubs do you think this is sufficient for your concerns?

[JoseInTheArena]

Here's my interpretation of all this:

• github/eslint-plugin-github is used by both dotcom and Primer
• The monorepo within the monolith is linting using the ssr-friendly rules
• The ssr-friendly rules are also being used in PRC (albeit with not the same rules enabled)

And, here's some questions I think we should answer through this issue:

• Should Primer be using the same rules as dotcom? (e.g. I think the delta is ssr-friendly/no-dom-globals-in-constructor and ssr-friendly/no-dom-globals-in-react-cc-render)
• Should we consolidate around github/eslint-plugin-github and add ssr-friendly there so that that flows wherever that config gets extended?
• (while we're at it) ui/.eslintrc.js has a comment about it being a "temporary eslint config for all ui packages" and that it "should be moved to a central eslint-config-shared package and imported by each package individually". Is this happening already? What do we need to do to make it so if it isn't?

cc: @dgreif as I believe that comment about the ui config being temporary is yours

[dgreif]

Regarding ui/.eslintrc.js, We should probably update that comment as we have added more to that config and made it a root, so it's no longer temporary. There were some bits added in there, like ssr-friendly, which we should move to the ui/packages/eslintrc packages. The downside to that move is that we have lots of code outside of ui/packages which isn't SSR friendly yet.

I'm honestly not sure we should add SSR rules to this base eslint-plugin-github. SSR is not something that everyone at GitHub or the broader community is worried about, so I don't think we want to push compatibility by default. If we can add it as an optional config, I'm for it, but I'm not sure it should be on by default. Weakly held opinion though!

[lesliecdubs]

Should Primer be using the same rules as dotcom? (e.g. I think the delta is ssr-friendly/no-dom-globals-in-constructor and ssr-friendly/no-dom-globals-in-react-cc-render)

I would generally think Primer and dotcom should be using the same rules.I see ssr-friendly/no-dom-globals-in-constructor and ssr-friendly/no-dom-globals-in-react-cc-render rules were set to "off" in https://github.com/primer/react/pull/3164/files, although it looks like we're also purposefully disabling these rules using the one-line eslint-disable option where we might need to. @colebemis is there a reason we want to maintain those rules in PRC as "off" by default?

As far as adding SSR rules to the base eslint-plugin-github, I don't have a strong opinion if SSR is not something everyone at GitHub will need to be concerned with. However, if we are expecting increased use of Alloy internally, I do wonder if a lack of linting on these items could increase potential for SSR compatibility bugs.