chore(fe): upgrade to nextjs12 blueprint4 node16 #3119

tihuan · 2022-08-17T22:24:34Z

Upgrade next and node-sass to the latest version #1705

Reviewers

Functional:
@seve @hthomas-czi @frano-m

Readability:
@atarashansky

Changes

Upgrade Next.js from 11 to 12
Upgrade Blueprint.js from 3 to 4. Changelog
Pin node from 14 to 16. UPDATE: Pinning to 16.14.2 to avoid GH Action lint error exit code 243. [Source]
Remove styled-components and use emotion completely (so we can use Next's fast SWC compiler and only have one css-in-js dependency)

Notable changes:

Next12 doesn't use Babel anymore and defaults to use its new SWC rust based compiler, which is supposedly 17x faster migration doc. So in order for jest test runner to work with TS files, we need to update jest's config files to use next/jest to compile TS files before jest consumes them
Blueprint3 to Blueprint4 upgrade was a pain. It includes removing node-sass as a direct dependency (which is now officially deprecated in favor of sass(dart-sass)) AND also install @vgrid/sass-inline-svg to process Blueprint SVG files. On top of that, because we define a custom theme via sass, I had to follow custom sass processing setup steps to overcome Blueprint's very limited support of theming. Currently the Blueprint team hasn't come up with a solution for official theming support, as shown in this comment. Given that @vgrid/sass-inline-svg actually still depends on node-sass, we might need to move away from Blueprint completely sooner than later to avoid indirectly depending on a deprecated package for too long
Migration from styled-components to emotion includes a small change of removing the usage of import { css } from 'styled-components', because @emotion/css outputs a class string instead of a styling function. So I just rewrote those instances with a styling function instead! See frontend/src/components/Collections/components/Dataset/common/style.ts for example

UPDATE: Thanks to Alec, we just had to change the import from { css } from 'styled' to { css } from '@emotion/react'

QA steps (optional)

RDEV:
https://thuang-next-12-frontend.rdev.single-cell.czi.technology/

Local:

Make sure your node is v16
Run rm -rf node_modules && npm i to install all new dependencies
Run the app via make local-sync OR if your local app connects to dev/staging BE, then just npm run dev
App should work and all the styles should look the same!

Notes for Reviewer

Tagging all the people that work on FE for visibility!

seve

we should also bump

single-cell-data-portal/frontend/Dockerfile

Line 2 in c85d9a4

FROM node:14

tihuan · 2022-08-17T22:32:39Z

Oh thank you! Done ✅

frano-m · 2022-08-18T05:19:26Z

Hey @tihuan woah, epic and brave 🌟!
Looks like there are few minor and possibly unwanted changes from upgrading blueprint.

At a glance the N+ tag colour is different. It looks like Figma has moved on since we originally implemented N+ tag and so the PR doesn't match the current design or current implementation. If the current Figma is the desired design, we should update all N+ tags for consistency (the differences at a glance are border radius, colour, font size and padding).
The close icon x on the selected terms should be white. The classname bp3-tag-remove inherited its colour, however in updated classname bp4-tag-remove the colour is now specified.
The filter menu search input placeholder "Search" is a slightly lighter colour than the original implementation.
BP MenuItem with prop icon appears to have wrapped an additional element around bpx-icon with classname bp4-menu-item-icon. This had added unwanted styles like margin-right and height. Each menu item is now 2px taller and 4px wider as a result.

I'd love to see this on rdev to test the new collections form. And that's it for now! Nice work!!

atarashansky · 2022-08-18T17:14:23Z

For the record, I think I got import { css } from "@emotion/react" to work in place of import { css } from "styled-components"

tihuan · 2022-08-18T17:17:23Z

Ooh I didn't think of using css from @emotion/react instead of @emotion/css! I'll use that now. Thanks, Alec!

tihuan · 2022-08-19T20:27:05Z

.gitignore

@@ -141,3 +141,6 @@ dmypy.json
 .envrc
 .env.ecr
 *.sqlc
+
+# Typescript
+tsconfig.tsbuildinfo


a quick research shows that tsbuildinfo is not worth checking in

vercel/next.js#30815

tihuan · 2022-08-19T20:27:32Z

frontend/.nvmrc

@@ -1 +1 @@
-v15
+v16.14.2


Pinning to avoid GH Action lint error exit code 243
See: https://stackoverflow.com/a/71892226/3120863

tihuan · 2022-08-19T20:27:54Z

frontend/Dockerfile


 # install dependencies first, in a different location for easier app bind mounting for local development
 # due to default /opt permissions we have to create the dir with root and change perms
 RUN mkdir /opt/node_app && chown node:node /opt/node_app
 WORKDIR /opt/node_app
+
+RUN apt-get update && apt-get install -y make wget \


copied from GenEpi's Dockerfile 😆

tihuan · 2022-08-19T20:28:35Z

frontend/Dockerfile

-# -- TODO, we should try turning this back on later.
-# USER node
-COPY package.json package-lock.json* ./
+USER node


we can enable this now, because we switch back to root before running ENTRYPOINT below, so the blast radius is a little smaller

tihuan · 2022-08-19T20:28:56Z

frontend/Makefile

@@ -16,8 +16,7 @@ init:

 .PHONY: lint
 lint:
-	npm run prettier-check
-	npm run lint
+	npm run prettier-check & npm run lint


run both commands in parallel for speed

tihuan · 2022-08-19T20:30:14Z

frontend/next.config.js

+        }
+      ),
+    },
+  },


I removed swcMinify: true, because it breaks the build

See: vercel/next.js#38436 (comment)

tihuan · 2022-08-19T20:34:18Z

frontend/src/components/common/Grid/components/ActionButton/index.tsx

@@ -12,12 +14,7 @@ export default function ActionButton({
  isAnchorButton = false,
  ...props /* Spread props to allow for data-test-id and button specific attributes e.g. "href", "target", or "disabled". */
 }: Props): JSX.Element {
-  return (
-    <StyledActionButton
-      as={isAnchorButton ? AnchorButton : undefined}


@emotion styled's as seems to only work with native DOM elements

E.g.,

const Button = styled.button` // ...styles ` <Button as="a" /> // renders <a /> const StyledAnchorButton = styled(AnchorButton)` // ...styles ` <StyledAnchorButton as="a" /> // still renders <button /> and will throw TS error

tihuan · 2022-08-19T20:37:31Z

frontend/src/pages/docs/[...slug].tsx

-  h4,
-  h5,
-  h6 {
+  & h2,


@seve I got a stylelint error saying &h1 needs to go after h2, h3, h4, etc., but I assume that you wanted & for all header tags?

tihuan · 2022-08-19T20:38:34Z

frontend/src/views/WheresMyGene/components/HeatMap/style.ts

@@ -40,12 +40,12 @@ export const YAxisWrapper = styled.div`
  top: 0;
  left: 0;
  z-index: 1;
-  // Somehow Firefox requires this to scroll
+  /* Somehow Firefox requires this to scroll */


// comment style doesn't work in emotion

tihuan · 2022-08-19T20:39:17Z

frontend/src/views/WheresMyGene/components/HeatMap/utils.ts

@@ -229,7 +229,10 @@ interface CreateYAxisOptionsProps {
 /**
 * Used to calculate text pixel widths. Should be only created once.
 */
-const CTX = document.createElement("canvas").getContext("2d");
+const CTX =


nextjs 12 somehow invokes this file on the server side, which doesn't have access to document, so we need to check typeof document now

tihuan · 2022-08-19T20:40:14Z

frontend/tsconfig.json

@@ -19,7 +19,8 @@
    "target": "esnext",
    "types": ["jest-playwright-preset", "expect-playwright", "jest"],
    "forceConsistentCasingInFileNames": true,
-    "resolveJsonModule": true
+    "resolveJsonModule": true,
+    "incremental": true


this is enabled by nextjs, which speeds up TS compilation

https://www.typescriptlang.org/tsconfig#incremental

tihuan · 2022-08-19T20:41:23Z

I'm going through the UI one more time after Fran's comment to see if I can catch more issues 😆

tihuan · 2022-08-22T17:42:48Z

Hey @tihuan woah, epic and brave 🌟! Looks like there are few minor and possibly unwanted changes from upgrading blueprint.

At a glance the N+ tag colour is different. It looks like Figma has moved on since we originally implemented N+ tag and so the PR doesn't match the current design or current implementation. If the current Figma is the desired design, we should update all N+ tags for consistency (the differences at a glance are border radius, colour, font size and padding).

Wow I didn't even notice the border radius changed from 2px to 1px 🤯 , how did you spot that? Too cool!

It seems to me that the font size and padding of N+ tag are the same as before?

And since the border radius and color change are both part of the Blueprint4 design, I wonder if it's okay to keep them as is!

@hthomas-czi PTAL 🙏

The close icon x on the selected terms should be white. The classname bp3-tag-remove inherited its colour, however in updated classname bp4-tag-remove the colour is now specified.

The filter menu search input placeholder "Search" is a slightly lighter colour than the original implementation.

BP MenuItem with prop icon appears to have wrapped an additional element around bpx-icon with classname bp4-menu-item-icon. This had added unwanted styles like margin-right and height. Each menu item is now 2px taller and 4px wider as a result.

Thank you so much for catching this 🙏 Updated to match the current design!

I'd love to see this on rdev to test the new collections form. And that's it for now! Nice work!!

The rdev is now available! https://thuang-next-12-frontend.rdev.single-cell.czi.technology
And I've compared this branch to dev, and the diff seems to be just the gray color as intended in Blueprint 4 design for better a11y color contrast, so I'm thinking maybe we should keep that? Let's see what @hthomas-czi thinks!

Thanks again for the wonderful review, Fran! I really appreciate your insights here!

tihuan · 2022-08-22T17:43:42Z

The PR should be ready for everyone to review now! Thanks again!

tihuan · 2022-08-24T17:44:07Z

frontend/src/pages/docs/[...slug].tsx


 const ImageContainer = styled.div`
  width: 100%;
  margin: 24px 0;

-  > div {
+  > span {


this change is needed since Next12 changed the Image wrapper from div to span:

https://nextjs.org/docs/upgrading#nextimage-changed-wrapping-element

Images look good now! Thanks so much for catching this, @seve 🙏

atarashansky

Readability is good, I have no suggestions other than a couple small questions. I'll wait for @seve to approve.

atarashansky · 2022-08-24T17:50:48Z

frontend/src/pages/_document.tsx


 const OG_PAGE_TITLE = "Cellxgene Data Portal";

 export default class Document extends RawDocument {
-  static async getInitialProps(


Just curious, why is this function going away?

Ah sorry for missing to document this change!

styled-components needed the extra config to server side render the stylesheets, but @emotion handles it automatically, so less code for us to maintain!

atarashansky · 2022-08-24T17:51:54Z

frontend/src/pages/docs/[...slug].tsx

@@ -275,7 +279,7 @@ const StyledLeftNav = styled.div`
  ::-webkit-scrollbar-thumb {
    background-clip: padding-box;
    border-right: 4px #f8f8f8 solid;
-    background: grey;
+    background-color: grey;


gray vs grey? are both recognized?

ooh never thought about that 💡 !

Just confirmed that they are both recognized!

https://www.w3schools.com/colors/colors_names.asp

atarashansky · 2022-08-24T17:53:27Z

frontend/src/views/WheresMyGene/components/HeatMap/utils.ts

@@ -229,7 +229,10 @@ interface CreateYAxisOptionsProps {
 /**
 * Used to calculate text pixel widths. Should be only created once.
 */
-const CTX = document.createElement("canvas").getContext("2d");
+const CTX =


tihuan · 2022-08-24T18:12:26Z

Thanks so much, @seve and @atarashansky !

I'll also wait for @hthomas-czi and @frano-m for approval before merging 🚀

tihuan · 2022-08-24T18:29:43Z

Harley approved in Slack!

* fix: remove self example from openapi (#3173) * chore: cherry pick changes from #3172 surface dataset batch_condition in curation API (#3177) * feat: surface dataset batch_condition in curation API * resolve merge conflict * fix: document upload links that are suported (#3176) * Document the types of links that are supported * Update documentation * docs: Renaming scExpression to Gene Expression (#3091) * docs: Renaming scExpression to Gene Expression - changing route to gene-expression instead of scExpression * chore(fe): upgrade to nextjs12 blueprint4 node16 (#3119) * chore(fe): upgrade to nextjs12 blueprint4 node16 * upgrade packages to resolve vulns * fix(lint): lint stuff * fix(lint): lint stuff * restore css usage * update Dockerfile * clean up css * fix bp4 css change * update docsite image css * fix: remove access_type from curation API (#3080) * remove access_type form curation API * remove is_allowed check from reshape_for_curation_api * update openapi * Fix the error message when curator is used by not a super curator * fix errors * Move all reshaping collection code into reshape_for_curation_api. * fix: change the datasets response shape (#3178) change name to title in dataset change is_primary_data to list of boolean in dataset * fix(curation api):Remove h5ad suffix requirements (#3151) - remove .h5ad suffix from uploads and curator tags - check for embedded UUID in curator tag * fix: backend/Makefile local db improvements, migration README (#3029) * fix: backend/Makefile local db improvements, migration README - Rename the targets for loading data and loading schema into local test db - Adapt the commands for the above two scripts to actually work (??) * doc updates * additional doc update * style: remove unused regex and consolidate curator tag logic (#3200) Co-authored-by: Trent Smith <1429913+Bento007@users.noreply.github.com> Co-authored-by: Nayib Gloria <55710092+nayib-jose-gloria@users.noreply.github.com> Co-authored-by: ashin-czi <109984998+ashin-czi@users.noreply.github.com> Co-authored-by: Timmy Huang <tihuan@users.noreply.github.com>

* fix: remove self example from openapi (#3173) * chore: cherry pick changes from #3172 surface dataset batch_condition in curation API (#3177) * feat: surface dataset batch_condition in curation API * resolve merge conflict * fix: document upload links that are suported (#3176) * Document the types of links that are supported * Update documentation * docs: Renaming scExpression to Gene Expression (#3091) * docs: Renaming scExpression to Gene Expression - changing route to gene-expression instead of scExpression * chore(fe): upgrade to nextjs12 blueprint4 node16 (#3119) * chore(fe): upgrade to nextjs12 blueprint4 node16 * upgrade packages to resolve vulns * fix(lint): lint stuff * fix(lint): lint stuff * restore css usage * update Dockerfile * clean up css * fix bp4 css change * update docsite image css * fix: remove access_type from curation API (#3080) * remove access_type form curation API * remove is_allowed check from reshape_for_curation_api * update openapi * Fix the error message when curator is used by not a super curator * fix errors * Move all reshaping collection code into reshape_for_curation_api. * fix: change the datasets response shape (#3178) change name to title in dataset change is_primary_data to list of boolean in dataset * fix(curation api):Remove h5ad suffix requirements (#3151) - remove .h5ad suffix from uploads and curator tags - check for embedded UUID in curator tag * fix: backend/Makefile local db improvements, migration README (#3029) * fix: backend/Makefile local db improvements, migration README - Rename the targets for loading data and loading schema into local test db - Adapt the commands for the above two scripts to actually work (??) * doc updates * additional doc update * style: remove unused regex and consolidate curator tag logic (#3200) Co-authored-by: Trent Smith <1429913+Bento007@users.noreply.github.com> Co-authored-by: Nayib Gloria <55710092+nayib-jose-gloria@users.noreply.github.com> Co-authored-by: ashin-czi <109984998+ashin-czi@users.noreply.github.com> Co-authored-by: Timmy Huang <tihuan@users.noreply.github.com> Co-authored-by: Trent Smith <1429913+Bento007@users.noreply.github.com> Co-authored-by: Nayib Gloria <55710092+nayib-jose-gloria@users.noreply.github.com> Co-authored-by: ashin-czi <109984998+ashin-czi@users.noreply.github.com> Co-authored-by: Timmy Huang <tihuan@users.noreply.github.com>

tihuan requested review from MillenniumFalconMechanic, seve, atarashansky, frano-m and ashin-czi August 17, 2022 22:24

tihuan self-assigned this Aug 17, 2022

tihuan changed the title ~~Thuang 1705 upgrade next 12~~ chore(fe): upgrade to nextjs12 blueprint4 node16 Aug 17, 2022

seve requested changes Aug 17, 2022

View reviewed changes

tihuan force-pushed the thuang-1705-upgrade-next-12 branch from ede4449 to b028fdc Compare August 17, 2022 22:31

tihuan force-pushed the thuang-1705-upgrade-next-12 branch 6 times, most recently from a3e7e29 to 7d7922f Compare August 19, 2022 20:23

tihuan commented Aug 19, 2022

View reviewed changes

tihuan force-pushed the thuang-1705-upgrade-next-12 branch from 570b80b to 288a727 Compare August 19, 2022 20:40

tihuan force-pushed the thuang-1705-upgrade-next-12 branch from 234243f to 1d4a8a4 Compare August 22, 2022 17:28

tihuan requested a review from hthomas-czi August 22, 2022 17:43

tihuan force-pushed the thuang-1705-upgrade-next-12 branch 2 times, most recently from e70609f to 998c713 Compare August 23, 2022 17:05

tihuan requested a review from seve August 24, 2022 17:20

tihuan force-pushed the thuang-1705-upgrade-next-12 branch from c8c57af to de7399d Compare August 24, 2022 17:21

tihuan commented Aug 24, 2022

View reviewed changes

atarashansky reviewed Aug 24, 2022

View reviewed changes

seve approved these changes Aug 24, 2022

View reviewed changes

tihuan added 9 commits August 24, 2022 14:45

chore(fe): upgrade to nextjs12 blueprint4 node16

a425469

upgrade packages to resolve vulns

926cd99

fix(lint): lint stuff

57228d0

fix(lint): lint stuff

dc45c04

restore css usage

fa191c2

update Dockerfile

30fa42d

clean up css

4ca7ae5

fix bp4 css change

fe5547e

update docsite image css

c09c4e5

tihuan force-pushed the thuang-1705-upgrade-next-12 branch from 2835c88 to c09c4e5 Compare August 24, 2022 21:45

Merge branch 'main' into thuang-1705-upgrade-next-12

fb05698

tihuan enabled auto-merge (squash) August 25, 2022 20:25

tihuan merged commit 5798bf9 into main Aug 25, 2022

tihuan deleted the thuang-1705-upgrade-next-12 branch August 25, 2022 20:34

danieljhegeman mentioned this pull request Sep 1, 2022

chore: deploy prod <- staging #3221

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(fe): upgrade to nextjs12 blueprint4 node16 #3119

chore(fe): upgrade to nextjs12 blueprint4 node16 #3119

tihuan commented Aug 17, 2022 •

edited

seve left a comment

tihuan commented Aug 17, 2022

frano-m commented Aug 18, 2022

atarashansky commented Aug 18, 2022

tihuan commented Aug 18, 2022

tihuan Aug 19, 2022

tihuan Aug 19, 2022

tihuan Aug 19, 2022

tihuan Aug 19, 2022

tihuan Aug 19, 2022

tihuan Aug 19, 2022

tihuan Aug 19, 2022 •

edited

tihuan Aug 19, 2022

tihuan Aug 19, 2022

tihuan Aug 19, 2022

atarashansky Aug 24, 2022

tihuan Aug 19, 2022

tihuan commented Aug 19, 2022

tihuan commented Aug 22, 2022

tihuan commented Aug 22, 2022

tihuan Aug 24, 2022

tihuan Aug 24, 2022

atarashansky left a comment

atarashansky Aug 24, 2022

tihuan Aug 24, 2022

atarashansky Aug 24, 2022

tihuan Aug 24, 2022

atarashansky Aug 24, 2022

tihuan commented Aug 24, 2022

tihuan commented Aug 24, 2022

		@@ -1 +1 @@
		v15
		v16.14.2

chore(fe): upgrade to nextjs12 blueprint4 node16 #3119

chore(fe): upgrade to nextjs12 blueprint4 node16 #3119

Conversation

tihuan commented Aug 17, 2022 • edited

Reviewers

Changes

QA steps (optional)

Notes for Reviewer

seve left a comment

Choose a reason for hiding this comment

tihuan commented Aug 17, 2022

frano-m commented Aug 18, 2022

atarashansky commented Aug 18, 2022

tihuan commented Aug 18, 2022

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 Aug 19, 2022 • edited

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 Aug 19, 2022

tihuan commented Aug 22, 2022

tihuan commented Aug 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atarashansky 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

tihuan commented Aug 24, 2022

tihuan commented Aug 24, 2022

tihuan commented Aug 17, 2022 •

edited

tihuan Aug 19, 2022 •

edited