[Proposal] Superset Frontend Style Guidelines

[etr2460]

The following is a proposal for high level frontend style guidelines, distilled from the many past SiPs and proposals. These high level guidelines will be the cover page for Superset's Frontend Style Guide, and we'll propose further Dos and Don'ts that conform to these guidelines and provide more actionable details for contributors. This proposal will be posted to the dev@ email list for lazy consensus before being migrated to the Superset Wiki.

---

Proposed Superset Frontend Style Guidelines

This is list of statements that describe how we do frontend development in Superset. While they might not be 100% true for all files in the repo, they represent the gold standard we strive towards for frontend quality and style.

• We develop using TypeScript.
• We use React for building components, and Redux to manage app/global state.
• We prefer functional components to class components, and use hooks for local component state.
• We use Ant Design components from our component library whenever possible, only building our own custom components when it's required.
• We use @emotion to provide styling for our components.
  • We co-locate styling within component files.
• We use Jest for unit tests, React Testing Library for component tests, and Cypress for end to end tests.
• We add tests for every new component or file added to the frontend.
• We organize our repo so similar files live near each other, and tests are co-located with the files they test.
• We prefer small, easily testable files and components.
• We use ESLint and Prettier to automatically fix lint errors and format the code.
  • We do not debate code formatting style in PRs, instead relying on automated tooling to enforce it.
  • If there’s not a linting rule, we don’t have a rule!

References

[SIP-36] Proposal for standardizing use of TypeScript

This SIP formally adopted TypeScript as the main language for Superset frontend, started migration work for JavaScript => TypeScript, and added the requirement that all new frontend JavaScript should be written in TypeScript instead.

[SIP-37] Proposal to implement CSS-in-JS using Emotion

This SIP formally adopted Emotion as the method for adding styling to components, describes alternatives considered, and why migration is valuable.

[SIP-48] Using Ant Design as our primary component library

This SIP replaced Bootstrap with Ant Design as our main component library, and describes how they should interact with style overrides provided by Emotion.

[SIP-56] Adopt React Testing Library for testing React components

This SIP replaced Enzyme with React Testing Library for testing React components, primarily because RTL is the most used testing framework and fully supports the newest React features.

[SIP-61] Improve the organization of front-end folders

This SIP describes how we organize our frontend folders, including how we should group different types of modules and components, where tests should be located, and how files should be named.

to: @rusackas @michael-s-molina @ktmud @graceguo-supercat @eschutho @suddjian @nytai @pkdotson @betodealmeida and anyone else who works on the frontend that I may have missed!

[ktmud]

This is a great starting point! I like If there’s not a linting rule, we don’t have a rule!, but I think some places would still benefit from more specific rules? For example:

• React props should go with the component itself, and not in a types.ts file
• Don't put multiple components in a React file (should be capturable by eslint)
• Use type ... for React props and interface ... for all other places
• Prefer TypeScript string literals type over Enums and const variables
• How to use variables in Emotion

These are hard to capture in linter and formatter but could still help code maintainability, readability and performance.

[graceguo-supercat]

thanks for bringing a Proposal. could you highlight some points that is in currently daily development, but preferred to be or will be changed in the further by this proposal?

[michael-s-molina]

Do we want to have some more specific style guide, too?

@ktmud That's the idea. In the end, what we want to achieve is a Wiki similar to this:

• Design Guidelines
• Front End Guidelines
  • Typescript
  • Style/CSS/Theming
  • Testing
  • Linting & Syntax
  • State Management
• Back End Guidelines

Each section/page should contain specific guidelines like this one already being cooked by @rusackas that falls into the Style/CSS/Theming page.

[etr2460]

could you highlight some points that is in currently daily development, but preferred to be or will be changed in the further by this proposal?

@graceguo-supercat it's my understanding that everything in this proposal is already currently agreed to by the team and part of daily development. Let me know if that's incorrect!

[eschutho]

This is great @etr2460! I'm not sure if you want feedback on further development of some of these points, since this is high-level, but on the Redux vs local state with hooks bullet point for example, I assume we'll want to flush that out more soon. Like when is it considered "component" vs "global"? That gray area where local state is being passed around between 2-3 sub components... where does that fall? What do we do with api requests that are only used in one component... should those all be stored in redux even if only used in one component? These are some things that some of us are currently thinking about. :)

[etr2460]

Yup @eschutho , totally agree about clarifying when to use Redux vs. local state. I think we'll probably write a whole dos and don'ts for that question, although it's a bit too low level for this summary proposal.

That said, if you asked me to chime in, i'd probably say:

• state used by only one component should always be local, even if it's coming from an api (since it's probably good to have updated state in cases where a component is unrendered then rerendered)
• state used by more than one component should always be in Redux (I feel less strongly here, and could see arguments either way, but having a hard and fast rule might make our lives easier and prevent that grey area from existing)

I'm not a huge fan of Redux anyway, so i'll leave the decision making regarding state to those with more experience, more passion, and less angst 😅

[michael-s-molina]

where does that fall?

@eschutho @etr2460 People with experience and interest in Redux can write a proposition of dos and don'ts similar to what @rusackas did for Style/CSS here. Then submit it for lazy consensus. After approval, it will become a page under Front End Guidelines. I think this will be a good process for all the topics.

[eschutho]

@etr2460 and @michael-s-molina thank you both for that feedback. A few of us are going to be working on cleaning up Redux state over the next few months, so we'll also start to write down some guidelines as we go along and share them out to the community to iterate on.