Introduce app-root #5423

goplayoutside3 · 2023-09-28T02:19:38Z

Package

app-root

Linked Issue and/or Talk Post

FEM packages structure discussion: #5089
Toward: #5429

Describe your changes

Create app-root package
Use Next.js 13 App Router for /about, /users, /groups/ and /projects
Setup Grommet theme and styled-components stylesheet
Import ZooHeader and ZooFooter into layout.js

How to Review

app-root can be run locally with yarn dev

Please read through ADR 54 to check for relevant information and typos.

eatyourgreens · 2023-09-28T08:27:57Z

packages/app-root/.gitignore

Why is there a .gitignore file?

There are .gitignore files in app-project, app-content-pages, and lib-classifier. Should there not be one here?

I’m not sure. This one seems to be mostly duplicating the one at the root of the monorepo.

goplayoutside3 · 2023-10-02T02:58:37Z

Okay I think I solved the FOUC! I ended up creating a test project outside of the monorepo workspace that included the same dependencies and file structure here. During that process, I finally saw a warning that Grommet is incompatible with styled-components v6, so I bumped the version back to v5.

@eatyourgreens when you have a minute, could you take a look at this branch locally, try yarn dev and see if a FOUC is visible? I think it'd be helpful to double check this branch on more than one machine.

I plan to add an ADR to this PR before marking it ready for review, and will elaborate on next steps needed for app-root in #5429.

coveralls · 2023-10-02T03:02:21Z

coverage: 82.153%. remained the same when pulling 0647d78 on app-root into 057599d on master.

packages/app-root/next.config.js

packages/app-root/package.json

packages/app-root/src/components/RootLayout.js

eatyourgreens · 2023-10-02T09:19:33Z

packages/app-root/src/app/style-registry.js

@@ -0,0 +1,27 @@
+'use client'


Interesting, so the server sends an unstyled page to the browser, then styles are built in the client?

Or maybe not. The downloaded HTML includes an inline stylesheet. Vendor prefixes can definitely be removed here.

If we could move all this into an external CSS file, that would cost us less to build and serve, but I don't think that's possible with styled components and Next.js. At the moment, the server has to rebuild the CSS whenever the page is rebuilt (#3380), so whenever content changes. However, the CSS only changes once a week, when we deploy.

👍 this stylesheet fixes the page styling issue.

Breakpoints should be 48rem, not 768px, but that's probably a bug in the Grommet theme.

This is odd. I built the app a second time. After the second build, I don't see the server-side stylesheet in the HTML, so it's unstyled again when it first loads.

Screen.Recording.2023-10-02.at.12.59.12.mov

Seems ok after running the bootstrap script and building for a third time. 🤷‍♂️

Good to know, thanks for double checking!

eatyourgreens · 2023-10-02T09:20:58Z

packages/app-root/src/app/style-registry.js

+    return <>{styles}</>
+  })
+
+  if (typeof window !== 'undefined') return <>{children}</>


This checks if the code is running in a browser, but the file specifies use client, so that's confusing.

I'm also a bit confused. I used the example from https://nextjs.org/docs/app/building-your-application/styling/css-in-js#styled-components, but don't totally understand why this line is included.

Good to know I'm not the only one that's confused by how this works. 😄

eatyourgreens · 2023-10-02T09:21:38Z

packages/app-root/src/app/style-registry.js

+  // x-ref: https://reactjs.org/docs/hooks-reference.html#lazy-initial-state
+  const [styledComponentsStyleSheet] = useState(() => new ServerStyleSheet())
+
+  useServerInsertedHTML(() => {


This hook name is confusing in a client-side component.

Although this is a client component, you noted earlier the downloaded HTML includes an inline stylesheet. Client components still run once on the server, and then again in the browser. This code is copied from the styled-components example: https://nextjs.org/docs/app/building-your-application/styling/css-in-js#styled-components

Do you recommend changing the function name?

The hook's name is set by Next.js, so that's fine. I was just confused as to how this component works.

packages/app-root/src/app/style-registry.js

eatyourgreens · 2023-10-02T09:37:34Z

80k of JS in the browser before we've even begun to build anything. 😞

Next.js build report for the stub app, with the client-side bundle already at 80k even though the page content is HTML-only.

EDIT: those must be gzipped sizes. Next.js is actually shipping 600k of client JS!

The browser dev console, showing JS download sizes for the new app, in the network panel.

eatyourgreens · 2023-10-02T09:41:36Z

I'm running this locally, and the links in ZooHeader are all external links to www.zooniverse.org. For this app, you'll need to build your own navigation menu, using next/link for the links that are handled by Next.js.

eatyourgreens · 2023-10-02T09:47:11Z

The local app uses HTTP. At some point we'll also need to add a custom dev server to handle HTTPS, so that we can authenticate to Panoptes in development.

EDIT: we'll need a Dockerfile too, so that we can build and deploy. Nothing urgent, but making sure that we've got these on our radar so that we don't miss them out.

eatyourgreens · 2023-10-02T10:57:09Z

I think the large bundle size is my fault. I removed the module entry point for @zooniverse/react-components in #5325. That probably broke tree-shaking in Webpack. This PR is probably exporting the whole library in the bundle, even though it only uses ZooHeader and ZooFooter.

#5430 might fix it.

eatyourgreens · 2023-10-02T11:51:23Z

I've published @zooniverse/react-components 1.6.3, which might lower the bundle size. If not, you can import just the components that you need from version 1.6:

import ZooHeader from '@zooniverse/react-components/ZooHeader'
import ZooFooter from '@zooniverse/react-components/ZooFooter'

In Next.js 13.5, you can also try optimising package imports in next.config.js.

goplayoutside3 · 2023-10-03T02:23:52Z

Re these points:

I'm running this locally, and the links in ZooHeader are all external links to www.zooniverse.org. For this app, you'll need to build your own navigation menu, using next/link for the links that are handled by Next.js.

The local app uses HTTP. At some point we'll also need to add a custom dev server to handle HTTPS, so that we can authenticate to Panoptes in development.

I've added them to the running list in #5429 which might be converted to a project board 👍

Co-authored-by: Jim O'Donnell <jim@zooniverse.org>

eatyourgreens · 2023-10-03T16:06:41Z

Changes here look good if the aim of this PR is to get the basic theme styles working. 👍

goplayoutside3 · 2023-10-04T01:59:41Z

Changes here look good if the aim of this PR is to get the basic theme styles working. 👍

Cool! I'm going to add documentation to the Readme and an ADR. I'll change this to ready for final review in the next 24 hrs.

docs/arch/adr-54.md

eatyourgreens · 2023-10-05T07:56:57Z

Sorry, one other comment. Have you any thoughts on how this app will use the staging vs. production Panoptes API?

At the moment, production projects are served from /projects/production and staging from /projects/staging. We rewrite request URLs to point to the appropriate path in the Next.js build, depending on whether the request is for a Panoptes staging project or a Panoptes production project.

front-end-monorepo/packages/app-project/middleware.js

Lines 57 to 62 in 61804c0

    
             /* 
        
               Project pages are served from /projects/staging/[owner]/[project] 
        
               and /projects/production/[owner]/[project] 
        
             */ 
        
             url.pathname = `/${panoptesEnv}${pathname}` 
        
             return NextResponse.rewrite(url)

Obviously, with this app, we'll have staging/production users too. Also a staging API homepage and a production API homepage. On the homepage, I think that Featured Projects is the only place where the Panoptes API would be used on the server. All the other Panoptes requests are made by the browser (I think.)

packages/app-root/package.json

packages/app-root/src/app/layout.js

eatyourgreens · 2023-10-05T09:18:10Z

packages/app-root/jsconfig.json

What does this file do?

Oh, is it a Visual Studio thing?

Correct! I'll delete this file

Uh oh, this file is actually responsible for resolving paths. When I delete it there's an error:

Module not found: Can't resolve '@/components/RootLayout' > 1 | import RootLayout from '@/components/RootLayout'

packages/app-root/src/app/layout.js

eatyourgreens · 2023-10-05T09:30:35Z

I've got a lot of questions, but I don't think there's anything that would stop this from merging. What do you think?

eatyourgreens · 2023-10-05T14:44:25Z

I ran a Lighthouse report in Chrome, and it scored us low on performance, because there's so much JS. Total JS execution time is ~3s on a mobile device.

Lighthouse scores for the new app. Performance 65, Accessibility 100, Best Practices 100, SEO 95.

eatyourgreens · 2023-10-05T14:48:37Z

The weird thing is, there's nothing in the pages that uses JS. If you disable JS in your browser, then load http://localhost:3000, the new home page loads just fine, with CSS styles inlined into the page head.

eatyourgreens · 2023-10-05T15:51:04Z

I downgraded #5411 to Next 13.5.3 and it still won't build the content pages storybook, so the problem might be the latest update to Storybook (7.4.6.)

eatyourgreens · 2023-10-05T16:05:05Z

I'm going to split #5411 into one PR that fixes the build warnings for Next 13.4, and a second PR that upgrades to Next 13.5.

goplayoutside3 · 2023-10-05T16:33:19Z

I ran @next/bundle-analyzer locally with app-root and here's the output. I'm not surprised to see Grommet and React as large libraries, but I am surprised to see i18next included. Does the fact that app-root lives in a monorepo affect bundle size here?

Node.js

Client

@eatyourgreens in your network screenshot, what is the significance of "transferred" versus "size"?

goplayoutside3 · 2023-10-05T16:44:28Z

Oh wait, I just answered my own question. Even though i18next isn't used directly in app-root, ZooHeader and ZooFooter use it for translations. And that's also why the translation json dictionaries are in the client screenshot above.

eatyourgreens · 2023-10-05T16:47:32Z

@eatyourgreens in your network screenshot, what is the significance of "transferred" versus "size"?

I think Next.js gzips by default, in which case that's showing gzipped size vs. actual size.

goplayoutside3 · 2023-10-05T16:53:18Z

I think Next.js gzips by default, in which case that's showing gzipped size vs. actual size.

Thanks!

It's interesting that I see different sizes when using Firefox versus Chrome. For instance, when I build and run app-root locally with Firefox, I see the exact screenshot you posted above.

However, this is the size displayed by an Incognito window in Chrome:

goplayoutside3 · 2023-10-05T16:56:20Z

Ah actually, it may just be a naming difference. In the bottom bar, Chrome "resources" is the actual size and "transferred" is the gzipped. Whereas in Firefox, "Size" is actual size and it's displayed directly next to total "transferred".

goplayoutside3 · 2023-10-05T16:59:47Z

So, just for my understanding - We're concerned about actual size of ~600kb versus gzipped ~200kb because the browser still has to uncompress the gzipped content to the size of 600kb and send it to the user?

eatyourgreens · 2023-10-05T17:06:19Z

The browser downloads ~170kB, then unzips the received file, then runs it. So it's running 600kB of JS, which is why Lighthouse has a poor performance score. There's also a performance hit because unzipping takes time too.

eatyourgreens · 2023-10-05T17:07:12Z

We should turn off gzip in production, because our problems with the CDN in the past were caused by FrontDoor trying to gzip resources that were already gzipped.

eatyourgreens · 2023-10-05T17:18:30Z

Nice to see that imports from @zooniverse/react-components are tiny. I think that means that recent changes to support tree-shaking and modularise that library are paying off in terms of smaller bundles.

eatyourgreens · 2023-10-05T17:19:55Z

I wonder why react-dom.production.min.js is being bundled and downloaded twice. That seems wrong. Also, agreed about how much i18next contributes to bundle size!

EDIT: react-dom is being bundled from node_modules/next/dist/compiled/react-dom and also from node_modules/react-dom. I knew Next has its own bundled copy of React, but I didn't realise it bundled it up like this.

goplayoutside3 · 2023-10-05T17:30:38Z

Agreed it's frustrating react is bundled twice! As for i18next, the whole library is imported in i18n.js in lib-react-components as part of its config.

I'm not going to address either of those issues in this PR, but will definitely bring it up during next week's standup and/or document it 👍

eatyourgreens · 2023-10-05T17:34:06Z

We could alias react to next/dist/compiled/react (and the same for react-dom.) Seems like a hack though. 🤔

goplayoutside3 added 5 commits September 20, 2023 18:55

init app and basic homepage

28c39fc

bring in ZooHeader and ZooFooter

a6c405f

wip

ef0cbcc

Merge branch 'master'

1646fca

remove grommet container for now

495aa02

eatyourgreens reviewed Sep 28, 2023

View reviewed changes

goplayoutside3 added 3 commits October 1, 2023 20:17

add styled-components registry

cc8bf03

bump styled-components down to v5

75180ee

Merge branch 'master'

c6355b1

eatyourgreens reviewed Oct 2, 2023

View reviewed changes

packages/app-root/next.config.js Outdated Show resolved Hide resolved

eatyourgreens reviewed Oct 2, 2023

View reviewed changes

packages/app-root/package.json Outdated Show resolved Hide resolved

eatyourgreens reviewed Oct 2, 2023

View reviewed changes

packages/app-root/src/components/RootLayout.js Outdated Show resolved Hide resolved

eatyourgreens reviewed Oct 2, 2023

View reviewed changes

packages/app-root/src/app/style-registry.js Outdated Show resolved Hide resolved

goplayoutside3 and others added 2 commits October 2, 2023 21:33

disableVendorPrefixes in styled-components registry

b258ac7

Co-authored-by: Jim O'Donnell <jim@zooniverse.org>

Merge branch 'master'

afcb090

suggestions from PR review

9ad5bad

bump app-root node version back to other apps' version (18)

41f1913

eatyourgreens reviewed Oct 5, 2023

View reviewed changes

docs/arch/adr-54.md Outdated Show resolved Hide resolved

eatyourgreens reviewed Oct 5, 2023

View reviewed changes

packages/app-root/package.json Outdated Show resolved Hide resolved

packages/app-root/package.json Outdated Show resolved Hide resolved

packages/app-root/src/app/layout.js Outdated Show resolved Hide resolved

eatyourgreens reviewed Oct 5, 2023

View reviewed changes

packages/app-root/src/app/layout.js Outdated Show resolved Hide resolved

eatyourgreens approved these changes Oct 5, 2023

View reviewed changes

github-actions bot added the approved This PR is approved for merging label Oct 5, 2023

update ADR 54

1c02628

Merge branch 'master'

98dc8e1

add bundle analyzer for now, can be removed

eaef472

change bundle analyzer to a dev dependency

0647d78

goplayoutside3 merged commit c9cc4f1 into master Oct 5, 2023
7 checks passed

goplayoutside3 deleted the app-root branch October 5, 2023 18:32

Introduce app-root #5423

Introduce app-root #5423

Conversation

goplayoutside3 commented Sep 28, 2023 • edited Loading

Package

Linked Issue and/or Talk Post

Describe your changes

How to Review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goplayoutside3 commented Oct 2, 2023

coveralls commented Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

eatyourgreens Oct 2, 2023 • edited Loading

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

eatyourgreens commented Oct 2, 2023 • edited Loading

eatyourgreens commented Oct 2, 2023 • edited Loading

eatyourgreens commented Oct 2, 2023 • edited Loading

eatyourgreens commented Oct 2, 2023 • edited Loading

eatyourgreens commented Oct 2, 2023 • edited Loading

goplayoutside3 commented Oct 3, 2023

eatyourgreens commented Oct 3, 2023

goplayoutside3 commented Oct 4, 2023

eatyourgreens commented Oct 5, 2023 • edited Loading

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

eatyourgreens commented Oct 5, 2023

eatyourgreens commented Oct 5, 2023

eatyourgreens commented Oct 5, 2023

eatyourgreens commented Oct 5, 2023

eatyourgreens commented Oct 5, 2023

goplayoutside3 commented Oct 5, 2023 • edited Loading

goplayoutside3 commented Oct 5, 2023 • edited Loading

eatyourgreens commented Oct 5, 2023

goplayoutside3 commented Oct 5, 2023

goplayoutside3 commented Oct 5, 2023 • edited Loading

goplayoutside3 commented Oct 5, 2023

eatyourgreens commented Oct 5, 2023

eatyourgreens commented Oct 5, 2023

eatyourgreens commented Oct 5, 2023

eatyourgreens commented Oct 5, 2023 • edited Loading

goplayoutside3 commented Oct 5, 2023 • edited Loading

eatyourgreens commented Oct 5, 2023

goplayoutside3 commented Sep 28, 2023 •

edited

Loading

coveralls commented Oct 2, 2023 •

edited

Loading

eatyourgreens Oct 2, 2023 •

edited

Loading

eatyourgreens commented Oct 2, 2023 •

edited

Loading

eatyourgreens commented Oct 2, 2023 •

edited

Loading

eatyourgreens commented Oct 2, 2023 •

edited

Loading

eatyourgreens commented Oct 2, 2023 •

edited

Loading

eatyourgreens commented Oct 2, 2023 •

edited

Loading

eatyourgreens commented Oct 5, 2023 •

edited

Loading

goplayoutside3 commented Oct 5, 2023 •

edited

Loading

goplayoutside3 commented Oct 5, 2023 •

edited

Loading

goplayoutside3 commented Oct 5, 2023 •

edited

Loading

eatyourgreens commented Oct 5, 2023 •

edited

Loading

goplayoutside3 commented Oct 5, 2023 •

edited

Loading