Upgrade the public app to Next v10 and new architectural patterns #1087

jaredcwhite · 2021-03-24T21:00:01Z

Part of the work for #1081

Cypress test issue on CircleCI: see #1087 (comment)

dominikx96

@jaredcwhite just a few comments, more quality improvements than nextjs update, overall looks good! 👍

sites/public/pages/_app.tsx

sites/public/pages/index.tsx

sites/public/pages/listing/[id]/[slug].tsx

dominikx96 · 2021-03-29T08:10:37Z

sites/public/src/translations.ts

+  es: spanishTranslations,
+  zh: chineseTranslations,
+  vi: vietnameseTranslations,
+} as Record<string, any>


Here we can use language enum as a type.

dominikx96 · 2021-03-29T08:11:37Z

ui-components/src/navigation/LanguageNav.tsx

@@ -24,7 +24,9 @@ const LanguageNav = ({ items }: LanguageNavProps) => {
            {items?.map((item) => (
              <li
                key={item.prefix}
-                onClick={() => changeLanguage(item.prefix)}
+                onClick={() => {
+                  void router.push(router.asPath, router.asPath, { locale: item.prefix || "en" })


Also, language enum would be helpful :)

jaredcwhite · 2021-03-29T20:02:33Z

Thanks @dominikx96 for the feedback. I'm a little fuzzy on what we would use a language enum for. The keys are coming from the Next.js config which comes from an ENV var, and any deployment with different vars would need code changed manually. Can you explain your thinking further? Thanks!

jaredcwhite · 2021-03-29T20:34:56Z

I just realized I totally forgot to update the listings page! On it…

jaredcwhite · 2021-03-29T21:24:42Z

Also need to refactor out the use of the lRoute helper…and LocalizedLink

jaredcwhite · 2021-04-02T23:52:15Z

Also need to refactor out the use of the lRoute helper…and LocalizedLink

So…this is most definitely going to be a breaking change 🚨 to ui-components, requiring an update of all Next apps. Not a problem per se, but we'd obviously need to be aware for the housingbayarea branches. A package version bump there will need the whole Next v10.1 upgrade migrated over.

ui-components is now website framework-agnostic

emilyjablonski · 2021-05-04T17:13:01Z

sites/partners/pages/listings/applications/add.tsx

@@ -1,8 +1,9 @@
 import React from "react"
 import Head from "next/head"
-import { MetaTags, PageHeader, SiteAlert, t } from "@bloom-housing/ui-components"


Why was MetaTags removed from ui-components?

MetaTags was using the next/head dependency, so in order to remove Next from the component package we'll need to handle anything in the HTML <head> through Next in the site folders.

emilyjablonski · 2021-05-04T17:14:01Z

sites/public/lib/ApplicationConductor.ts

@@ -205,13 +205,12 @@ export default class ApplicationConductor {
  }

  routeTo(url: string) {
-    void Router.push(url).then(() => window.scrollTo(0, 0))


What was the point of the scrollTo logic before?

It was going to a new page but not scrolling up to the top of the page by default. But in Next v10 they added that functionality automatically (sort of bonkers it wasn't there from the get-go).

emilyjablonski · 2021-05-04T17:14:50Z

sites/public/next.config.js

          sassLoaderOptions: {
            additionalData: tailwindVars,
          },
-          // exportPathMap adapted from https://github.com/zeit/next.js/blob/canary/examples/with-static-export/next.config.js


Is all of this automatically generated now?

Yep, that's the beauty of the updated system. No routing information needed in the config file whatsoever! 🤩

@jaredcwhite will we be able to hide routes that we don't want live yet?

emilyjablonski · 2021-05-04T17:18:32Z

sites/public/layouts/application.tsx

-          <LocalizedLink href="/listings" className="navbar-item">
-            {t("nav.listings")}
-          </LocalizedLink>
+          <Link href="/listings">


Feels a little weird to see a with a href wrapping an a tag without one, but I don't necessarily have a suggestion

That's just how Next's link component works — although because I had to add a special LinkComponent wrapper for pushing into the NavigationContext for the components layer, it's possible we might decide to use LinkComponent ourselves too. But that'd be a "non-standard" pattern as far as Next is concerned.

Here's the doc for reference: https://nextjs.org/docs/api-reference/next/link, though it doesn't seem like it's necessary since it's not done below on lines 74 and 75.

emilyjablonski · 2021-05-04T17:26:44Z

Could you provide some additional context in the PR description / an overview of what was done here?

seanmalbert

Hey @jaredcwhite ,
Again, this looks good overall. Added a couple more comments, for things we may want to do in the future.

seanmalbert · 2021-05-06T22:59:08Z

sites/public/pages/listings.tsx

+      return nowTime > moment(listing.applicationDueDate)
+    })
+  } catch (error) {
+    console.error(error)


Here's another general question that I don't think we want to solve here (if there's anything to change). Can we incorporate an error boundary, so that we don't have to have these individual try catches, and we can let errors bubble up to a single handler? That would help with logging in general.

Yeah, I feel like there could be (and should be, if not already) some kind of best practice Next recommends for handling errors across an application. I'll look into it further.

seanmalbert · 2021-05-06T23:11:15Z

sites/public/src/ListingView.tsx

+      </ListingDetails>
+    </article>
+  )
+}


This feels like it could be broken up into smaller components to be used by this. Not saying we need to do that now, there's just a lot going on in this file.

dominikx96 · 2021-05-07T11:29:46Z

@jaredcwhite when I tried to open one of existing listings I see an error:

jaredcwhite · 2021-05-07T22:12:06Z

@jaredcwhite when I tried to open one of existing listings I see an error:

@dominikx96 Hmm, as far as I can tell there's no case where the getStaticPaths function is called without a context object. Can you make sure you've yarn installed and try again?

dominikx96 · 2021-05-10T07:52:33Z

@jaredcwhite I removed node_modules, re-installed and checked again, works as expected! 👍

* Updated next packages * Updated next config file * Refactored application entry point and created translation file * Updated pages structure to use dynamic next ID * Added LinkComponent and updated _app component * Updated internal links to the partners pages * Updated hh-member form to show address fields only if they are defined * Cleanup * Added unique useSWR keys * Updated keys to use endpoint naming Co-authored-by: dominikx96 <[email protected]>

seanmalbert · 2021-05-18T13:53:18Z

@jaredcwhite , @dominikx96 and @software-project , is there any reason why the translation header patch #1244 can't be started with a branch of this after the merge conflicts are addressed?

jaredcwhite · 2021-05-18T16:48:05Z

@seanmalbert Yeah I'm not planning to enhance any additional functionality at this point, just trying to this PR to feature parity with master so it's mergeable.

jaredcwhite · 2021-05-18T20:15:22Z

Going to work through the testing issues now.

netlify · 2021-05-18T22:27:03Z

Deploy Preview for clever-edison-cd22c1 ready!

Built with commit 3abd0dc

https://deploy-preview-1087--clever-edison-cd22c1.netlify.app

jaredcwhite · 2021-05-18T22:48:28Z

All right – other than a couple of failing CircleCI tests which seem to be related to frontend/backend API connection (not sure how to resolve), we seem to be in good shape! Both public and partners building on Netlify, ui-components tests pass.

dominikx96 · 2021-05-19T10:42:32Z

@jaredcwhite I noticed this issue few times using commands from the main package.json (dev:all, dev:app:public, dev:app:partners, build:app:public, build:app:partners) - but now it works for me as expected. It sounds like the issue comes randomly until the backend is not ready (?), besides it looks good!

software-project · 2021-05-19T15:43:25Z

ui-components/src/actions/LocalizedLink.tsx

-  }
+// Legacy use only, deprecated
+const LocalizedLink = (props: PropsWithChildren<LinkProps>) => {
+  const { LinkComponent } = useContext(NavigationContext)


Very nice :)

jaredcwhite · 2021-05-19T23:06:07Z

@seanmalbert and I tried to iterate a bit on why Cypress is failing on CircleCI. Sean verified he's able to get all the tests to pass locally. I suspect at this point it has something to do with the backend not being loaded yet when the yarn build:app:public command it run in setup, and prior to Next v10 that sort of worked and now it falls down hard. We've decided to merge this in for now so it's not blocking PRs and then try to get the Cypress setup working again shortly.

Upgrade the public app to Next v10 and new architectural patterns

f3c3582

exygy-dev temporarily deployed to bloom-refere-next-10-up-apuhar March 24, 2021 21:00 Inactive

jaredcwhite added the wip This PR is not ready for review, do not review it's a “Work In Progress” label Mar 24, 2021

kathyccheng requested review from dominikx96 and software-project March 25, 2021 17:23

dominikx96 requested changes Mar 29, 2021

View reviewed changes

Update to Next 10.1 and related refactors

de6e4ca

exygy-dev temporarily deployed to bloom-refere-next-10-up-wzneaz April 2, 2021 18:35 Inactive

jaredcwhite mentioned this pull request Apr 10, 2021

Remove next router from LanguageNav component #1117

Closed

Merge branch 'master' into next-10-upgrades

807e2f9

exygy-dev temporarily deployed to bloom-refere-next-10-up-irymmy April 26, 2021 23:52 Inactive

jaredcwhite mentioned this pull request Apr 27, 2021

Move Link/Router components to React Context #1185

Closed

Add NavigationContext to remove Next.js dependency

ace344a

ui-components is now website framework-agnostic

exygy-dev temporarily deployed to bloom-refere-next-10-up-46we70 April 30, 2021 20:35 Inactive

Use static/server context types instead of any

18735e9

exygy-dev temporarily deployed to bloom-refere-next-10-up-oscu4n April 30, 2021 20:54 Inactive

Further abstract out LinkComponent

24419b5

exygy-dev temporarily deployed to bloom-refere-next-10-up-cqqscd May 3, 2021 20:38 Inactive

Merge branch 'master' into next-10-upgrades

5f802b2

exygy-dev temporarily deployed to bloom-refere-next-10-up-fk8rpd May 3, 2021 21:19 Inactive

A few some changes to Partners sites

6dc7de8

exygy-dev temporarily deployed to bloom-refere-next-10-up-41ewhu May 3, 2021 22:29 Inactive

jaredcwhite changed the title ~~WIP: Upgrade the public app to Next v10 and new architectural patterns~~ Upgrade the public app to Next v10 and new architectural patterns May 3, 2021

jaredcwhite removed the wip This PR is not ready for review, do not review it's a “Work In Progress” label May 3, 2021

emilyjablonski reviewed May 4, 2021

View reviewed changes

exygy-dev temporarily deployed to bloom-refere-next-10-up-taetlc May 5, 2021 23:20 Inactive

Use consistent footer links in layout

69d953b

exygy-dev temporarily deployed to bloom-refere-next-10-up-mrk70w May 5, 2021 23:31 Inactive

kathyccheng mentioned this pull request May 6, 2021

Translate Next Update into Partners #1211

Closed

seanmalbert reviewed May 6, 2021

View reviewed changes

dominikx96 self-requested a review May 7, 2021 09:45

seanmalbert mentioned this pull request May 12, 2021

Next JS follow on - contextual axios instance with translation headers and county headers housingbayarea/bloom#287

Closed

seanmalbert mentioned this pull request May 18, 2021

Patch of Next JS for translation issues #1244

Closed

jaredcwhite added 2 commits May 18, 2021 10:47

Merge branch 'master' into next-10-upgrades

ca74932

Use native Next language methods

b64dba2

jaredcwhite added 4 commits May 18, 2021 14:10

Set working LinkComponent and fix tests in ui-components

99e51de

Fix failing ImageCard test

2c75d7b

Update Netlify deploy settings

3dab09b

Set Next target to serverless for proper Netlify rollout

ed59895

software-project reviewed May 19, 2021

View reviewed changes

fixes public build

3abd0dc

jaredcwhite merged commit b87eff3 into master May 19, 2021

This was referenced May 24, 2021

Next.js upgrade for best practices in SSG/SSR, i18n, and more #1081

Closed

Limit redirect to only relative URLs #1162

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade the public app to Next v10 and new architectural patterns #1087

Upgrade the public app to Next v10 and new architectural patterns #1087

jaredcwhite commented Mar 24, 2021 •

edited

Loading

dominikx96 left a comment

dominikx96 Mar 29, 2021

dominikx96 Mar 29, 2021

jaredcwhite commented Mar 29, 2021

jaredcwhite commented Mar 29, 2021

jaredcwhite commented Mar 29, 2021 •

edited

Loading

jaredcwhite commented Apr 2, 2021

emilyjablonski May 4, 2021

jaredcwhite May 4, 2021

emilyjablonski May 4, 2021

jaredcwhite May 4, 2021

emilyjablonski May 4, 2021

jaredcwhite May 4, 2021

software-project May 19, 2021

emilyjablonski May 4, 2021

jaredcwhite May 4, 2021

seanmalbert May 4, 2021

emilyjablonski commented May 4, 2021

seanmalbert left a comment

seanmalbert May 6, 2021

jaredcwhite May 7, 2021

seanmalbert May 6, 2021 •

edited

Loading

jaredcwhite May 7, 2021

dominikx96 commented May 7, 2021

jaredcwhite commented May 7, 2021

dominikx96 commented May 10, 2021

seanmalbert commented May 18, 2021 •

edited

Loading

jaredcwhite commented May 18, 2021

jaredcwhite commented May 18, 2021

netlify bot commented May 18, 2021 •

edited

Loading

jaredcwhite commented May 18, 2021

dominikx96 commented May 19, 2021 •

edited

Loading

software-project May 19, 2021

jaredcwhite commented May 19, 2021

Upgrade the public app to Next v10 and new architectural patterns #1087

Upgrade the public app to Next v10 and new architectural patterns #1087

Conversation

jaredcwhite commented Mar 24, 2021 • edited Loading

dominikx96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredcwhite commented Mar 29, 2021

jaredcwhite commented Mar 29, 2021

jaredcwhite commented Mar 29, 2021 • edited Loading

jaredcwhite commented Apr 2, 2021

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

emilyjablonski commented May 4, 2021

seanmalbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanmalbert May 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominikx96 commented May 7, 2021

jaredcwhite commented May 7, 2021

dominikx96 commented May 10, 2021

seanmalbert commented May 18, 2021 • edited Loading

jaredcwhite commented May 18, 2021

jaredcwhite commented May 18, 2021

netlify bot commented May 18, 2021 • edited Loading

jaredcwhite commented May 18, 2021

dominikx96 commented May 19, 2021 • edited Loading

Choose a reason for hiding this comment

jaredcwhite commented May 19, 2021

jaredcwhite commented Mar 24, 2021 •

edited

Loading

jaredcwhite commented Mar 29, 2021 •

edited

Loading

seanmalbert May 6, 2021 •

edited

Loading

seanmalbert commented May 18, 2021 •

edited

Loading

netlify bot commented May 18, 2021 •

edited

Loading

dominikx96 commented May 19, 2021 •

edited

Loading