thuang-765-next-js #926

tihuan · 2021-02-22T19:10:58Z

#765

Live link: https://thuang-next-js-frontend.rdev.single-cell.czi.technology/

Reviewers

Functional:
@seve

Readability:
@MDunitz

Changes

add

Add favicon support
Add basic SSR for static layout
Add _app.tsx and _document.tsx per Nextjs for adding global stylesheets and other global stuff
Add next/head for modifying page.title instead of react-helmet

remove

A few dead dependencies in package.json, on top of Gatsby ones
Remove @reach/router and use Nextjs's own router for prefetching pages doc
Remove theme-ui

modify

Migrate from Gatsby to Next.js
Change port from Gatsby's default 8000 to 3000. However, Jessica did say that rdev prod build still needs to run on :9000 for historical reasons 🙆‍♂️
Minimally restructure the app to work with Next.js's file system based routing. Which means every route needs its own file in /pages
Upgrade a few dependencies, since NPM was complaining about incompatible versions
Use next/image for Image optimization

tihuan · 2021-02-22T19:11:56Z

DEV_ENV.md

@@ -1,61 +1,71 @@
 # Local Development Environment


prettier change and changing Frontend app port from 8000 to 3000

tihuan · 2021-02-22T19:33:55Z

docker-compose.yml

@@ -1,4 +1,4 @@
-version: '3.8'


prettier change and s/8000/3000

tihuan · 2021-02-22T19:34:15Z

frontend/.gitignore

@@ -58,16 +58,16 @@ typings/
 # configs


Nextjs and Gatsby need different .gitignore config

tihuan · 2021-02-22T19:35:00Z

frontend/babel.config.js

@@ -0,0 +1,13 @@
+module.exports = {


Babel config is now needed, since Nextjs doesn't use the plugin paradigm that Gatsby uses. This setting works along side with https://github.com/styled-components/babel-plugin-styled-components#readme

tihuan · 2021-02-22T19:36:47Z

frontend/container_init.sh

 else
-    npm run build
-    exec gatsby serve --host 0.0.0.0
+  exec npm run build-and-start-prod -p 9000


do prod build and serve

tihuan · 2021-02-22T23:12:03Z

frontend/src/views/Collection/components/DatasetUploadToast/index.tsx


-const DatasetUploadToast = Toaster.create({


.create() actually queries and inserts a new element in the DOM, which doesn't work for SSR. So I changed this to only call .create() when .show() is called on the client side

tihuan · 2021-02-22T23:12:48Z

frontend/src/views/Collection/index.tsx

+  let id = "";
+  let isPrivate = false;
+
+  if (Array.isArray(params)) {


This logic handles frontend/src/pages/collections/[...params].tsx

tihuan · 2021-02-22T23:13:42Z

frontend/src/views/Collection/index.tsx

-            uploadedFiles={uploadedFiles}
-            invalidateCollectionQuery={invalidateCollectionQuery}
-            onSelect={setSelected}
+    <>


The only change here is adding a wrapper <></> and a <Head /> sibling to <ViewGrid />

tihuan · 2021-02-22T23:14:52Z

frontend/tsconfig.json

@@ -5,19 +5,40 @@
    "esModuleInterop": true,


prettier changes and Nextjs config added some more TS related settings

tihuan · 2021-02-22T23:15:20Z

scripts/setup_dev_data.sh

@@ -4,7 +4,7 @@ export AWS_DEFAULT_REGION=us-west-2
 export AWS_ACCESS_KEY_ID=nonce


prettier change and s/8000/3000

tihuan · 2021-02-22T23:27:14Z

frontend/package.json

  },
  "devDependencies": {
+    "@babel/preset-react": "^7.12.13",


these two babel plugins are needed for jest to work in frontend/jest/jest-preprocess.js

tihuan · 2021-02-23T01:14:08Z

smoke-test is currently failing I think because of the temp local-docker.js config file temporarily setting API_URL to be: "http://backend:5000", which the DNS doesn't resolve properly 🤔

I'll ping Infra team tomorrow about this! Thank you!

tihuan · 2021-03-01T20:04:40Z

frontend/tests/features/homepage.test.ts

+    });
+
+    await Promise.all([
+      page.waitForNavigation(),


we need to wait for navigation before asserting the URL. Otherwise this test will be flakey!

codecov · 2021-03-26T20:12:30Z

Codecov Report

Merging #926 (1ede997) into main (f023c56) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #926   +/-   ##
=======================================
  Coverage   94.28%   94.28%           
=======================================
  Files          73       73           
  Lines        4476     4476           
=======================================
  Hits         4220     4220           
  Misses        256      256

Flag	Coverage Δ
backend	`94.28% <100.00%> (ø)`
python	`94.28% <100.00%> (ø)`
unitTest	`94.28% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...t/backend/chalice/api_server/test_v1_collection.py	`97.08% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f023c56...1ede997. Read the comment docs.

tihuan · 2021-03-26T21:54:08Z

frontend/.gitignore

@@ -1,3 +1,5 @@
+# WARNING: PLEASE MAKE SURE TO KEEP THIS FILE IN SYNC WITH `.dockerignore`


this warning is very important!! Otherwise RDEV might not work, because . dockerignore might be ignoring things we no longer ignore in .gitignore.

In our case, I updated .gitignore to stop ignoring public/ folder, because Next.js uses it to build the app, but Gatsby used to require us to ignore that folder. But I didn't update .dockerignore accordingly, which caused RDEV deploy to not work partially

tihuan · 2021-03-26T21:54:48Z

frontend/container_init.sh

+  # runs `npm run build && npm run serve` under the hood,
+  # so we need to pass `-- -p 9000` to `npm run serve`, which
+  # will then call `next start -p 9000` correctly
+  exec npm run build-and-start-prod -- -- -p 9000


this basically tells Next.js to run on port 9000

tihuan · 2021-03-26T21:56:52Z

frontend/src/common/queries/collections.ts

@@ -45,7 +45,11 @@ async function fetchCollection(
  _: unknown,


small update to collection queries to be more resilient to unexpected input

tihuan · 2021-03-26T21:57:39Z

frontend/src/components/Collections/components/PublishCollection/index.tsx

@@ -1,6 +1,6 @@
 import { Button, H6, Intent } from "@blueprintjs/core";


Start using next/router

tihuan · 2021-03-26T21:58:31Z

frontend/src/components/common/Logo/index.tsx

  return (
    <LogoWrapper data-test-id="logo">
-      <StyledLogo size={small ? 100 : undefined} src={String(logo)} />


No need StyledLogo anymore, since we only have one size for logo

tihuan · 2021-03-26T22:00:23Z

frontend/src/configs/dev.js

@@ -1,5 +1,6 @@
 const configs = {
  API_URL: "https://api.cellxgene.dev.single-cell.czi.technology",
+  SENTRY_DEPLOYMENT_ENVIRONMENT: "development",


setting up Sentry to capture CSP violation. However, this is not set up to capture build info like Explorer does at the moment

tihuan · 2021-03-26T23:11:50Z

frontend/src/pages/_document.tsx

+          <link
+            rel="manifest"
+            href="/site.webmanifest"
+            crossOrigin="use-credentials"


We need crossOrigin here in order to send cookies along with the request to prevent redev from blocking and redirecting the request to Okta. Thanks to @jgadling for finding this solution 🎉

https://chanzuckerbergteam.slack.com/archives/C01C310V70X/p1616795763052000?thread_ts=1616795713.051900&cid=C01C310V70X

seve

LGTM!

seve · 2021-03-29T20:21:10Z

frontend/src/components/Collections/components/Grid/common/utils.ts

@@ -1,4 +1,4 @@
-import { memoize } from "lodash-es";
+import memoize from "lodash/memoize";


seve · 2021-03-29T20:25:14Z

frontend/src/components/Collections/components/Grid/components/Row/CollectionRow/index.tsx

-        >
-          {name}
-        </CollectionTitleText>
+        <Link href={`/collections/${id}${isPrivate ? "/private" : ""}`}>


This might be useful so we don't need to have redudent hrefs jsx-eslint/eslint-plugin-jsx-a11y#402 (comment)

tihuan force-pushed the thuang-765-next-js branch from 5b9a3ec to 13acc4a Compare February 22, 2021 22:51

tihuan commented Feb 22, 2021

View reviewed changes

tihuan force-pushed the thuang-765-next-js branch 3 times, most recently from 895b589 to 8da4424 Compare February 22, 2021 23:26

tihuan commented Feb 22, 2021

View reviewed changes

tihuan force-pushed the thuang-765-next-js branch from 8da4424 to 7fda4a6 Compare February 22, 2021 23:50

tihuan changed the title ~~thuang-765-next-js~~ [DRAFT until ECS clusters run on prod after March 13] thuang-765-next-js Feb 22, 2021

tihuan force-pushed the thuang-765-next-js branch 3 times, most recently from e93aa34 to 7b829be Compare March 1, 2021 20:04

tihuan commented Mar 1, 2021

View reviewed changes

tihuan force-pushed the thuang-765-next-js branch 4 times, most recently from 0be905b to b330db2 Compare March 25, 2021 23:57

tihuan marked this pull request as ready for review March 26, 2021 17:36

tihuan requested review from Bento007 and MDunitz March 26, 2021 17:36

tihuan changed the title ~~[DRAFT until ECS clusters run on prod after March 13] thuang-765-next-js~~ thuang-765-next-js Mar 26, 2021

tihuan mentioned this pull request Mar 26, 2021

thuang-fix-deps #1020

Closed

tihuan force-pushed the thuang-765-next-js branch from cdd2557 to 8e7b942 Compare March 26, 2021 20:17

tihuan commented Mar 26, 2021

View reviewed changes

tihuan requested review from seve and removed request for Bento007 March 29, 2021 19:56

seve approved these changes Mar 29, 2021

View reviewed changes

tihuan added 8 commits March 29, 2021 14:32

thuang-765-next-js

b882cf6

fix container deploy -p 9000

b0ff7a9

add manifest-src

e4a6dc0

fix CSP typo

9f09e09

update .dockerignore

b635771

update object order

2f67235

add crossorigin for manifest

2938a42

add eslint disable for Next.js Link ane nested <a />

16bcf03

tihuan force-pushed the thuang-765-next-js branch from 56932a4 to 16bcf03 Compare March 29, 2021 21:32

Merge branch 'main' into thuang-765-next-js

1ede997

tihuan merged commit d7bc0c2 into main Mar 29, 2021

tihuan deleted the thuang-765-next-js branch March 29, 2021 23:48

This was referenced Mar 30, 2021

Add Data Portal CSP and appropriate rules [after March 13] #766

Closed

Migrate frontend off of Gatsby to Next.js #765

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

thuang-765-next-js #926

thuang-765-next-js #926

tihuan commented Feb 22, 2021 •

edited

Loading

tihuan Feb 22, 2021

tihuan Feb 22, 2021

tihuan Feb 22, 2021

tihuan Feb 22, 2021

tihuan Feb 22, 2021

tihuan Feb 22, 2021

tihuan Feb 22, 2021

tihuan Feb 22, 2021

tihuan Feb 22, 2021

tihuan Feb 22, 2021

tihuan Feb 22, 2021

tihuan commented Feb 23, 2021

tihuan Mar 1, 2021

codecov bot commented Mar 26, 2021 •

edited

Loading

tihuan Mar 26, 2021

tihuan Mar 26, 2021

tihuan Mar 26, 2021

tihuan Mar 26, 2021

tihuan Mar 26, 2021

tihuan Mar 26, 2021

tihuan Mar 26, 2021

seve left a comment

seve Mar 29, 2021

seve Mar 29, 2021

		@@ -4,7 +4,7 @@ export AWS_DEFAULT_REGION=us-west-2
		export AWS_ACCESS_KEY_ID=nonce

		@@ -1,3 +1,5 @@
		# WARNING: PLEASE MAKE SURE TO KEEP THIS FILE IN SYNC WITH `.dockerignore`

		@@ -45,7 +45,11 @@ async function fetchCollection(
		_: unknown,

		@@ -1,6 +1,6 @@
		import { Button, H6, Intent } from "@blueprintjs/core";

		@@ -1,4 +1,4 @@
		import { memoize } from "lodash-es";
		import memoize from "lodash/memoize";

thuang-765-next-js #926

thuang-765-next-js #926

Conversation

tihuan commented Feb 22, 2021 • edited Loading

Reviewers

Changes

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 commented Feb 23, 2021

Choose a reason for hiding this comment

codecov bot commented Mar 26, 2021 • edited Loading

Codecov Report

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

seve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tihuan commented Feb 22, 2021 •

edited

Loading

codecov bot commented Mar 26, 2021 •

edited

Loading