akveo / styleguide

5 stars 5 forks source link

Revamp React & React Native guides #35

Open hssrrw opened 1 year ago

hssrrw commented 1 year ago

General

Class components vs Hooks

I'd say there should be a preference for functional components over class-based ones. Class components now considered a legacy API and it is recommended to use functions (see https://react.dev/reference/react/Component)

Formatting

From my experience, Prettier now looks like a de-facto standard approach to format TS/JS code (at least in React community). It has a quite opinionated set of rules which leaves little room for flame discussions. Also, it allows to fine-tune exactly those rules that usually discussed and decided in a team internally (like max line length, quote symbol type, tab length etc.) Do you think we can relax these requirements and explicitly mark that kind of rules as a recommendation? Also, I noticed, prettier is explicitly disabled in eslint config: https://github.com/akveo/styleguide/blob/64bcd8d166c12b57229485d4845dd88f0e3a84dd/.eslintrc.json#L12 In my opinion, prettier rules are easier to maintain. The formatter is way smarter and more flexible than the set of eslint-rules. I also agree with the sentiment that linting should find bugs, whereas formatters should format the code.

Ternary operators inside JSX

Do not use ternary operator when one of the return expressions is null. Instead, use Short Circuit Evaluation: {hasIcon && <Icon />}

Can we add a comment like "don't forget to convert string values to boolean when using short circuit evaluation in React Native"? Sometimes we may easily forget to escape string values and write something like {title && <Title text={title} />} instead of {!!title && ...} and get an error. I actually think in this case it's not a terrible idea to explicitly state that this part can be null. {title ? <Title text={title} /> : null}

... Put only render methods or single line JSX expressions inside ternary operator. {isAuthenticated ? renderAuthFlow() : <SignIn />}

Let's not encourage render methods/props and instead use components since it's easier to debug and profile in debugger or Flipper. Render functions are not visible in the component tree, while components create a distinct and meaningful part of the tree.

Props

Use single quotes ' for jsx attributes.

What do you think about leaving it for teams to decide? I noticed a lot of projects and open source code (including Meta/FB codebase) using double quote for props and single quote in other places.

Inline styles are forbidden. Use Stylesheet.create or its analogue.

I think, it's too overly strict and can actually lead to less performant code. Sometimes we can't use style sheet factory, for example, when the styles should be dynamic or depend on props and so on. Typical case is style={{ transform: [{ translateX: dynamicallyCalculatedValue }] }}. So, one can incorrectly assume that since it's "forbidden", they should use Stylesheet.create inside render cycle, which is more expansive than an inline object. Let's use less strict language here and mention this exception since it's a quite common one. Also, it should be moved somewhere else from Formatting section since it's absolutely not about code format. Maybe Syntax or General. I'd call it more like Best practices.

Syntax

Naming Style

I would suggest getting rid of mentioning "component" in the code and file names. First of all, in React everything is built with components. If we use the term "component" in some narrower meaning, e.g. as an equivalent of "dumb"/ stateless component, this distorts the actual meaning of this concept in React. However, if you feel like that, you still can mark particular type of components with prefixes like "container" or something. I think just naming the file exactly the same as its component is enough (UserAvatar.tsx, ProgressBar.tsx etc.)

Also, there's a conflict in this between TS guide: https://github.com/akveo/styleguide/blob/64bcd8d166c12b57229485d4845dd88f0e3a84dd/typescript/README.md?plain=1#L250 (see example users.api.tsx)

and React one: https://github.com/akveo/styleguide/blob/64bcd8d166c12b57229485d4845dd88f0e3a84dd/react/README.md?plain=1#L30

Component Structure

The whole section seems to be redundant due to class components being discouraged from being used. However, we can mark this section as "legacy" or "deprecated" for the time being.

TypeScript

Language Rules

@ts-ignore

Let's not only recommend avoiding using @ts-ignore but also recommend adding a TODO with explanation what could be done to remove this ignoring.

Objects

Use trailing commas whenever there is a line break between the final property and the closing brace.

I think it's more suitable for the Formatting section instead.

Switch statement

Let's add a new recommendation and an eslint rule: Use exhaustive switch statements (https://typescript-eslint.io/rules/switch-exhaustiveness-check/). TL;DR: code maintenance. Say, today you remember this particular switch can only handle just a couple of cases from your enum or a union. Tomorrow you don't. Now, there's an opportunity for new errors to slip in.

Other

How about discouraging as along with @ts-ignore and any? It usually belongs to utility type functions but not to the regular code. In my opinion, widely using as makes typing system less strict and more error-prone.

Should we think about creating separate typescript style guides for React and Angular? In general, I feel like React ecosystem tries to stick to functional programming more and more, while our current TypeScript guide is all about OOP. I'm not sure how that is relevant for Angular since I never used it.

hssrrw commented 1 year ago

I can create a PR for what I think is the most important from mentioned above.