Add style guides for TypeScript, React, and Jest #603
Conversation
This includes an example `tsconfig.json` (expressing how to configure TypeScript to be as strict as possible), and an `.eslintrc.json` and `.prettierrc` for auto-formatting and auto-fixing.
|
|
||
| [Sample](sample.js) | ||
|
|
||
| * Use [ESlint] and [Prettier] for auto-formatting and auto-fixing |
There was a problem hiding this comment.
Do ESLint and Prettier eliminate the needs for the other guidelines here? (trailing commas, ===, etc)
There was a problem hiding this comment.
My thought was that ESLint and Prettier enforce the style guide, but that it was still worth listing the things we care most about directly in the guide. Alternately we could treat the ESLint and Prettier configs as the definitive source, but I find them to be less digestable for someone coming to the style guides and wanting to quickly read over all the guidelines.
There was a problem hiding this comment.
Devil's advocate, @composerinteralia authored #606 today which removes the ruby guidelines in favor of just using and referencing standard.
I like the simplicity of that, and not having to maintain guidelines in both a configuration file and a textual guide.
Curious what others think as well.
There was a problem hiding this comment.
Based on #606 (comment) and the discussion in #610 it looks like we might be trending towards removing guidelines that we can't automate and don't feel strongly about. Linking those from here so we can continue the conversation when we condense the guides.
| @@ -0,0 +1,50 @@ | |||
| # React | |||
There was a problem hiding this comment.
I think maybe this should be in best-practices instead (it's not about formatting) but the different between those is fuzzy. I wonder if they should be merged at this point.
There was a problem hiding this comment.
I agree, almost everything that came to mind for React was a best-practices thing (since style is already covered by the TypeScript guide).
I would be in favor of merging style and best practices guides. It might be worth calling out style guidelines vs best practices where both exist, but it also allows for guidelines which might be in more of a gray area.
There was a problem hiding this comment.
I have opened an issue to discuss combining them: #610
|
This is great! Thank you for putting it together. It would be good to get some feedback from folks who have worked more on React/TypeScript projects. |
There was a problem hiding this comment.
This is amazing!! I don't have much time today to work through this, but I took a quick look. I agree with most everything I've read, including the React best practices.
I'm interested in comparing the ESLint, Prettier, and tsconfig setup to what we're using in React Reference App.
style/testing-jest/README.md
Outdated
| * Use [React Hooks Testing Library] for testing [React Hooks] | ||
| * Use [User Event] for simulating DOM events on React components under test | ||
| * Use [Fishery] for building factories | ||
| * Prefer placing test suite files alongside source files (e.g. `Thing.tsx` |
There was a problem hiding this comment.
A pattern I've seen more often is to put tests in ./__tests__/Thing.test.tsx. I like that pattern since it keeps tests a bit out of the way but also almost colocated, but I've also seen this and don't feel too strongly.
There was a problem hiding this comment.
I really like having the test files as siblings of the component they test, though that may be a result of how I'm used to searching for them.
One benefit of the __tests__ folder is that it does reduce noise in a directory where there are a lot of components. But I have also found it makes it a little harder to, at a glance, see if a component has a test suite.
I'm also open to either way - curious to hear others' thoughts on this!
There was a problem hiding this comment.
I too prefer the tests as siblings! I find that the specs are better maintained when they arent hidden off in a folder. It seems to elevate the tests to more of a first class concern, which imo they should be.
There was a problem hiding this comment.
I am of a third opinion that tests should not be alongside components at all and instead be in their own root level directory that mirrors the components directory. Basically how most Rails projects (along with many other languages and frameworks) treat tests.
Although the entire community seems to be more in favor of tests near components and we'd be against almost every project we're likely to work with. I also see John's point about tests being a first-class citizen and I agree, but I have not found them to be more or less maintained because they are near or far in the file system. That feels like it's been more of a cultural problem than a test placement one.
If these are the options, I would at least prefer they are out of the way a little in a sibling folder. I think it makes them easier to work with which could decrease the friction of maintain them.
There was a problem hiding this comment.
I have created #613 to continue this discussion
style/react/README.md
Outdated
| * Use [React Hooks] | ||
| * Use pre-built hooks when possible (e.g. [streamich/react-use]) | ||
| * Use [custom hooks] to encapsulate stateful logic outside a component | ||
| * Avoid [Forwarding Refs] |
There was a problem hiding this comment.
Is this saying to prefer passing a ref as a custom prop like inputRef instead of using forwardRef or to avoid passing refs altogether?
There was a problem hiding this comment.
My thought here is probably better summarized as Avoid nesting Forward Refs.
The guideline I'd like to include here is less about how you should pass a ref, and more around when their use might point to an overly complex design.
I have seen some cases where there are multiple levels of components forwarding a ref, and it becomes increasingly difficult to trace the ref from its assignment to its usage. Furthermore a ref forwarded multiple layers and used by some far-off component creates a coupling between the two components and also pollutes the component tree when debugging with the React dev tools.
These have just been my observations, curious to hear others' thoughts too.
style/react/README.md
Outdated
| * Prefer the `children` prop over [render props] | ||
| * Prefer using [TypeScript prop interfaces] over [PropTypes] | ||
| * Prefer the [short syntax] when using [Fragments] | ||
| * Prefer [React Contexts] over [Redux] |
style/react/README.md
Outdated
| * Use [Jest](../testing-jest/README.md) for unit testing | ||
| * Use [styled-components] for CSS-in-JS | ||
| * Use [Apollo] with [code generation] when working with [GraphQL] | ||
| * Use [React Router] for routing in SPAs |
There was a problem hiding this comment.
There's something tricky here. I think SPAs are often not called for. Many SPAs (including the one you and I are working on now) do not need to be SPAs. If they must be SPAs, then I think encouraging React Router makes sense. I'm only worried that this feels to me like there's some implicit encouragement of SPAs. Does that make sense?
On the other hand, if someone made the mistake of picking an SPA, maybe we
There was a problem hiding this comment.
This is similar to the css-in-js discussion above. I didn't mean to imply that we should build SPAs, just that if we are the routing solution of choice would be React Router.
But looking at this from the perspective of someone reading these guides for perhaps the first time without that context, it does feel like a recommendation towards SPAs as a default which shouldn't be our stance.
https://github.com/thoughtbot/react-suspenders/pull/1 handles the SPA case right now since it is based off create-react-app. Maybe that, too, should have an option for bootstrapping a non-SPA variant, perhaps using rails suspenders. Though I don't have the expertise using those two together 🙂
| [styled-components]: https://styled-components.com/ | ||
| [React Hooks]: https://reactjs.org/docs/hooks-overview.html | ||
| [custom hooks]: https://reactjs.org/docs/hooks-overview.html#building-your-own-hooks | ||
| [streamich/react-use]: https://github.com/streamich/react-use |
There was a problem hiding this comment.
Ooo thanks for this! I didn't know it existed.
style/testing-jest/README.md
Outdated
| * Use [React Hooks Testing Library] for testing [React Hooks] | ||
| * Use [User Event] for simulating DOM events on React components under test | ||
| * Use [Fishery] for building factories | ||
| * Prefer placing test suite files alongside source files (e.g. `Thing.tsx` |
There was a problem hiding this comment.
I am of a third opinion that tests should not be alongside components at all and instead be in their own root level directory that mirrors the components directory. Basically how most Rails projects (along with many other languages and frameworks) treat tests.
Although the entire community seems to be more in favor of tests near components and we'd be against almost every project we're likely to work with. I also see John's point about tests being a first-class citizen and I agree, but I have not found them to be more or less maintained because they are near or far in the file system. That feels like it's been more of a cultural problem than a test placement one.
If these are the options, I would at least prefer they are out of the way a little in a sibling folder. I think it makes them easier to work with which could decrease the friction of maintain them.
Co-authored-by: Edward Loveall <edward@thoughtbot.com>
* master: Add style guides for TypeScript, React, and Jest (#603)
This PR is a first attempt at creating style guides for TypeScript, React, and Jest. I added the three together (along with modifications to the JavaScript style guide) since they reference each other.
Tasks remaining:
General thoughts and questions:
Given that our contribution guidelines for this repository state thathttps://github.com/thoughtbot/react-suspenders/pull/1Decisions about which libraries to use should live in template projects such as Suspenders, some of my included recommendations for libraries may be better suited for a similar React / TypeScript projectstyle/testing-jestbut I'm not convinced that's the right naming convention. Should we consider having a generic testing guide as well as a style guide for each testing framework we use? Our testing guides are ruby/rails-centric #611Should the examplehttps://github.com/thoughtbot/react-suspenders/pull/1/files#diff-32ab45b0a7cbf0aa8200c5670863c486tsconfig.jsonfile live here, or would that also be better suited for a Suspenders-like project?