Request: Include SSR-related rules #440

Activity

1. JoseInTheArena commented on May 15, 2023

   

   JoseInTheArena

   on May 15, 2023

   More actions

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

   assigned

   erinnachen

   on May 15, 2023
3. erinnachen commented on May 15, 2023

   

   erinnachen

   on May 15, 2023

   Contributor

   More actions

   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?
4. JoseInTheArena commented on May 22, 2023

   

   JoseInTheArena

   on May 22, 2023

   More actions

   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
5. dgreif commented on May 23, 2023

   

   dgreif

   on May 23, 2023

   Contributor

   More actions

   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!
6. lesliecdubs commented on May 29, 2023

   

   lesliecdubs

   on May 29, 2023

   Author

   More actions

   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.

   👍React with 👍1Reacted by Dusty Greif