🧰 Frontend Infrastructure and Tooling 🛠️ #3

codemonkey800 · 2021-04-09T17:10:28Z

Description

This PR sets up the infrastructure and tooling for the napari hub frontend. The README includes a list of the tech we use:

From client/README.md

⚡ React + Next.js

⚔️ TypeScript

🎨 SCSS modules

💅 Tailwind CSS for utility styles

🏎️ Tailwind JIT for on-demand Tailwind styles

📦 Yarn for package management

📸 Jest + React Testing Library for unit and snapshot tests

🎭 Jest + Playwright for E2E tests

🔍 ESlint + Stylelint for TypeScript and SCSS linting

⚙️ Plop for boilerplate automation

References

Several frontend infra/tooling choices were inspired by decisions made in other CZI frontend repos:

CSS Modules in `edu-design-system`

In the beginning, the edu-design-system was originally using twin.macro for CSS-in-JS Tailwind styles. I experimented with twin.macro in the React POC and found the developer experience awesome. Later on the team switched to CSS modules for several reasons outlined in chanzuckerberg/edu-design-system#362. After consideration, I decided that CSS modules would be a good choice because:

Able to take advantage of full flexibility of SCSS.
More accessible for non-JavaScript experts.
Meaningful class names per element.
Easy to work with dynamic className using clsx package.
Zero-runtime vs emotion which has a runtime
No incredibly long Tailwind class name strings. For example: tw="flex flex-auto items-center justify-center lg:grid ..."
We can separate Tailwind classes by line and organize them based on functionality:

.appBar {
  @apply flex flex-auto;
  @apply items-center justify-center;
  @apply lg:grid ...;
}

Plop Generators in `edu-design-system`

The team has a plopfile.js file for generating components. This would be extremely valuable to have in the napari hub client because we can:

Standardize the component file structure.
Automate away the boilerplate for common routines.
Provide tooling for people less experienced with frontend or that are new to the repo.

E2E Testing in `corpora-data-portal`

The team setup the corpora-data-portal in a way that allows E2E tests with Playwright, and so the E2E test configuration for napari hub closely resembles the configuration used in the corpora-data-portal repo.

codemonkey800 · 2021-04-09T17:42:58Z

There are a lot of things to document here regarding the tooling and infrastructure that I didn't quite get to doing in this PR. I plan on writing up some documentation in a future PR detailing everything outlined here. Also, things might change during review, so I want to make sure I write documentation for what's already merged in 😄

codemonkey800 · 2021-04-09T17:58:19Z

client/tsconfig.json

+    "noUnusedLocals": true,
+    "noUnusedParameters": true,
+    "paths": {
+      "@/*": ["./src/*"]


This path mapping means that source code imports will be from @/*. For example, to import a component from a page:

import { AppBar } from '@/components';

Would love to hear opinions on this vs something like:

Tilde: ~/components or ~components

Relative: ../components

Dollar sign lib (similar to SvelteKit) : $lib/components

Src prefix: src/components

I decided to use the @/* prefix because:

It's immediately obvious that this import is from the source code and not from a package

Built in support for grouping and sorting using eslint-plugin-simple-import-sort

Fixes issue with relative import hell import { AppBar } from '../../components';

is this common practice, or is there a different standardized way of doing this?

is this common practice, or is there a different standardized way of doing this?

@kne42 I've outlined the most common formats above, but there isn't really a standardized practice for path mapping. I do know that several CZI projects use the src/* format:

https://github.com/chanzuckerberg/corpora-data-portal/blob/d7bc0c2aef8a09b311f1bb20c1755567dabcc4fa/frontend/tsconfig.json#L14-L16
https://github.com/chanzuckerberg/aspen/blob/0ecfd26e05883e6b6f61712ed96697ede606e6d8/src/ts/tsconfig.json#L20-L24

There is a way we can customize the grouping and support something like src/* or something, but I don't have any strong opinions.

What is nice about @/* is that it works with eslint-plugin-simple-import-sort out of the box, and it's 2 characters less to type than src/*.

Ah I think @ is used for scoped packages too, like from '@angular, from "@emotion, from "@material-ui/core/styles", etc.. So that might collide with npm packages as well!

My projects have been straight up using from "src/*" for autocomplete and avoid collision. But definitely don't have strong opinion on this!

UPDATE: I missed the forward slash in @/* 😆 NVM! That should work just fine!

codemonkey800 · 2021-04-09T18:10:49Z

client/README.md

@@ -8,7 +8,7 @@ cool frontend tech for the website:
 - :art: [SCSS modules](https://github.com/css-modules/css-modules)
 - :nail_care: [Tailwind CSS](https://tailwindcss.com/) for utility styles
 - :racing_car: [Tailwind JIT](https://tailwindcss.com/docs/just-in-time-mode) for on-demand Tailwind styles
- :package: [pnpm](https://github.com/pnpm/pnpm) for package management
+- :package: [Yarn](https://classic.yarnpkg.com/en/) for package management


While I love that pnpm is growing significantly and is up to 2x faster than competitors, it still doesn't have Dependabot support.

Dependabot has support for parsing and updating the yarn.lock file, which is crucial for detecting and fixing security vulnerabilities. When Dependabot gets support for the pnpm-lock.yaml files, we can revisit swapping Yarn for pnpm.

kne42

I don't really know anything about front-end infra so I just left a few questions/suggestions, feel free to ignore them 😅

client/tailwind.config.js

kne42 · 2021-04-09T19:39:35Z

client/tsconfig.json

+    "noUnusedLocals": true,
+    "noUnusedParameters": true,
+    "paths": {
+      "@/*": ["./src/*"]


is this common practice, or is there a different standardized way of doing this?

kne42 · 2021-04-09T19:45:32Z

Oh I guess one other thing, it would be great if you could link out to client/README from somewhere in the main README so that people looking at the repo can figure out how to set it up and stuff!

tihuan

This is really great! Thanks for setting this up, @codemonkey800 🎉 !

Just some minor comments, otherwise LGTM! 🔥 🚀 ✨ 🤩

tihuan · 2021-04-09T20:16:13Z

client/.vscode/extensions.json

@@ -0,0 +1,12 @@
+{


Ah we probably just need to add .vscode to .gitignore, so we don't commit /.vscode/*!

I actually left this in on purpose to suggest extensions for the project and also enforce that VSCode use the TypeScript version in node_modules. Any particular reasons why we shouldn't include it?

oh that's a great idea!! I didn't think of that 💡

My VSCode adds .vscode/settings.json to the project directory because I use https://marketplace.visualstudio.com/items?itemName=johnpapa.vscode-peacock to set different color for different projects, so I just ignored .vscode completely 😆

No other reasons other than that!

tihuan · 2021-04-09T20:20:28Z

client/src/components/Layout/Layout.tsx

+
+export function Layout({ children }: Props) {
+  return (
+    <>


nit: <></> doesn't seem needed, since we only have single child here?

Sure thing! This was actually a leftover from the POC that I must have forgotten to change. Will update it!

tihuan · 2021-04-09T20:28:31Z

client/src/components/Layout/Layout.test.tsx

+  });
+
+  it('should match snapshot', () => {
+    expect(component).toMatchSnapshot();


should we only capture the fragment for snapshot 😄?

E.g.,

const { asFragment } = component; expect(asFragment()).toMatchSnapshot();

Amazing, this shortens the snapshot file considerable. I'm actually new to snapshot testing, so thanks for the tip 🤩

tihuan · 2021-04-09T20:31:08Z

client/tailwind.config.js

+  JIT performance benefits .
+
+  TODO Remove this code when this issue is resolved:
+  https://github.com/tailwindlabs/tailwindcss-intellisense/issues/293


Looks like the issue is closed! tailwindlabs/tailwindcss-intellisense#293

Not sure if that's enough though:

This is fixed in v0.5.10 – note that this release does not include full support for the JIT engine. That is coming very soon though! 👀 For now the extension behaves as if you don't have JIT enabled.

I love open source, it moves so quickly. I'll take a look and see if this fixes our use case 😆

tihuan · 2021-04-09T20:32:35Z

client/tsconfig.jest.json

+{
+  "extends": "./tsconfig.json",
+  "compilerOptions": {
+    /*


comment doesn't work in JSON, I remember, so not sure if this will throw a syntax error?

The tsconfig.json file is special because it allows comments for some reason. For example, the sci-components repo includes most of the comments from running tsc --init:

https://github.com/chanzuckerberg/sci-components/blob/0707ce87184e36181cbabeae2a43d1b2c08e0555/tsconfig.json

Ahh TIL! Thanks for sharing! That's really awesome 🔥

tihuan · 2021-04-09T20:39:52Z

client/tsconfig.json

+    "noUnusedLocals": true,
+    "noUnusedParameters": true,
+    "paths": {
+      "@/*": ["./src/*"]


Ah I think @ is used for scoped packages too, like from '@angular, from "@emotion, from "@material-ui/core/styles", etc.. So that might collide with npm packages as well!

My projects have been straight up using from "src/*" for autocomplete and avoid collision. But definitely don't have strong opinion on this!

UPDATE: I missed the forward slash in @/* 😆 NVM! That should work just fine!

tihuan · 2021-04-09T20:49:07Z

client/.lintstagedrc.yml

+'*.{js,ts,tsx,scss,md,yml,yaml,json}':
+  - prettier --write
+'*.{js,ts,tsx}':
+  - eslint --fix


Personally on precommit, my projects have been only running prettier and let Github Action run linters on each PR. That way we only need to run linter locally if GH action fails the linter instead of running lint on every local commit!

E.g., https://github.com/chanzuckerberg/khepri/blob/develop/.github/workflows/tests.yml#L17

That sounds like a great idea now that I think about it. I was planning on having some GitHub actions for linting each PR too. Now that I think about it, we'd be linting every commit AND after the is released PR. So much time spent on linting when it could be done once when it's time for review. Occasionally I also find myself doing git commit --no-verify when doing a quick commit or while rebasing.

Thank you for the suggestion, I went ahead and updated the .lintstagedrc.yml file 😄

liu-ziyang · 2021-04-09T22:02:18Z

Having no context on the relationship with the edu design system, I want to ask about the reference and how dependent are we on their setup? I totally appreciate that you are able to pull existing resources to our advantage, just want to make sure this wouldn't be a constraint when we need to release the hub to the community

codemonkey800 · 2021-04-09T23:56:27Z

Having no context on the relationship with the edu design system, I want to ask about the reference and how dependent are we on their setup? I totally appreciate that you are able to pull existing resources to our advantage, just want to make sure this wouldn't be a constraint when we need to release the hub to the community

@ziyangczi No dependencies on anyone setup 😄 Everything we've used is all popular open source tooling used by many projects outside of CZI.

neuromusic · 2021-04-12T15:45:15Z

Everything we've used is all popular open source tooling

We should also consider getting Snyk setup for this repo. It monitors the repo for security vulnerabilities and license issues. CZI has an account & AFAIK the cellxgene, Meta, and IDseq teams all use it.

codemonkey800 · 2021-04-12T15:57:02Z

Everything we've used is all popular open source tooling

We should also consider getting Snyk setup for this repo. It monitors the repo for security vulnerabilities and license issues. CZI has an account & AFAIK the cellxgene, Meta, and IDseq teams all use it.

@neuromusic Ooh I've seen that around in the source code of other repos while working on this PR. I'd love to add it, but mind if we move forward with merging this PR and adding Synk in another PR?

If yes, should we create a GitHub issue or Clubhouse story to capture that work? 🤔

neuromusic · 2021-04-12T16:02:46Z

sorry, yes... didn't intend for that to block this PR

codemonkey800 · 2021-04-12T16:15:55Z

sorry for assuming! 😆

I wasn't sure if it was a request to add it to this PR or not, but thank you for the clarification 💯

tihuan · 2021-04-12T16:56:54Z

client/.vscode/extensions.json

+    "esbenp.prettier-vscode",
+    "jpoissonnier.vscode-styled-components",
+    "bradlc.vscode-tailwindcss"
+  ]


unblocker 😆 But highly recommends this spellchecker extension too!

https://marketplace.visualstudio.com/items?itemName=streetsidesoftware.code-spell-checker

I actually had to fix the location of the vscode settings too, so I went ahead and added it to the commit. I also use this extension 💯

tihuan · 2021-04-12T16:59:00Z

Everything we've used is all popular open source tooling

We should also consider getting Snyk setup for this repo. It monitors the repo for security vulnerabilities and license issues. CZI has an account & AFAIK the cellxgene, Meta, and IDseq teams all use it.

For Snyk, I remember some people were saying CZI ended Snyk contract already (or soon?), likely because Github now comes with free Dependabot checks. So definitely check with Infra to see what they recommend!

justinelarsen

Great work @codemonkey800 ! I don't have much feedback, but overall it looks good to me.

justinelarsen · 2021-04-12T18:12:53Z

client/tailwind.config.js

+      md: '495px',
+      lg: '600px',
+      xl: '875px',
+      '2xl': '1150px',


Love this, so organized and clean way of storing the breakpoints! I was wondering why '2xl' and '3xl' are in quotes and the rest aren't?

JavaScript will start parsing the 2xl and 3xl as numbers because of the leading 2 and 3, so we need to add quotes to make sure they're strings 😆

codemonkey800 force-pushed the jeremy/frontend-infra branch from 08a9dea to 6391670 Compare April 9, 2021 17:30

codemonkey800 added 16 commits April 9, 2021 10:30

Project dependencies

462bb39

Setup .nvmrc

62063bc

Node.js gitignore

e9d569b

Setup initial next.js app

584c7fa

Added SSR for React Query

2906f62

Added Tailwind CSS and JIT

3a08feb

Added script for generating types for SCSS modules

e6c6551

Setup Jest + React Testing Library for unit tests

225fffd

Setup Jest + Playwright for E2E tests

b53b4ed

Added prettier config

cc6cb86

Added stylelint config

a5f04a9

Added ESLint configs

7d21dfd

Added lint-staged and husky for linting on commit

b3bcb72

Added plopfile for component and page generators

57dd9ec

Added recommended VSCode extensions and settings

909cd0e

Added client README.md

8953850

codemonkey800 force-pushed the jeremy/frontend-infra branch from 6391670 to 8953850 Compare April 9, 2021 17:31

codemonkey800 commented Apr 9, 2021

View reviewed changes

Swap pnpm for yarn

5486951

codemonkey800 force-pushed the jeremy/frontend-infra branch from f0d2059 to 5486951 Compare April 9, 2021 18:06

codemonkey800 commented Apr 9, 2021

View reviewed changes

kne42 reviewed Apr 9, 2021

View reviewed changes

codemonkey800 requested review from tihuan and justinelarsen April 9, 2021 19:50

codemonkey800 added 2 commits April 9, 2021 13:18

Rename bg-napari-app-bar to bg-napari-primary

398b274

Added link to client/README.md in root README.md

c92b293

tihuan approved these changes Apr 9, 2021

View reviewed changes

codemonkey800 added 2 commits April 12, 2021 08:36

Minor refactors

1177252

Address review comments

520a10b

tihuan reviewed Apr 12, 2021

View reviewed changes

Fix vscode settings and add spell checker extension

1b838b3

justinelarsen reviewed Apr 12, 2021

View reviewed changes

justinelarsen approved these changes Apr 12, 2021

View reviewed changes

codemonkey800 merged commit 866d40f into chanzuckerberg:main Apr 12, 2021

codemonkey800 deleted the jeremy/frontend-infra branch April 12, 2021 18:34

codemonkey800 mentioned this pull request Apr 13, 2021

App Bar implementation #11

Merged

🧰 Frontend Infrastructure and Tooling 🛠️ #3

🧰 Frontend Infrastructure and Tooling 🛠️ #3

Conversation

codemonkey800 commented Apr 9, 2021 • edited Loading

Description

References

CSS Modules in edu-design-system

Plop Generators in edu-design-system

E2E Testing in corpora-data-portal

codemonkey800 commented Apr 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tihuan Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

codemonkey800 Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

kne42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kne42 commented Apr 9, 2021

tihuan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tihuan Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liu-ziyang commented Apr 9, 2021

codemonkey800 commented Apr 9, 2021

neuromusic commented Apr 12, 2021

codemonkey800 commented Apr 12, 2021

neuromusic commented Apr 12, 2021

codemonkey800 commented Apr 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tihuan commented Apr 12, 2021

justinelarsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codemonkey800 commented Apr 9, 2021 •

edited

Loading

CSS Modules in `edu-design-system`

Plop Generators in `edu-design-system`

E2E Testing in `corpora-data-portal`

tihuan Apr 9, 2021 •

edited

Loading

codemonkey800 Apr 9, 2021 •

edited

Loading

tihuan Apr 9, 2021 •

edited

Loading