NextJS #2146

lucaslcode · 2021-10-30T17:26:41Z

RIP

lucaslcode · 2021-11-04T15:51:26Z

Notes so far:
This isn't actually THAT many lines, git just didn't pick up some files as renames/moving location and the yarn.lock change is huge.

I think NextJS 12 is fine, I think it's working using the rust compiler (swc) for the app and babel for testing.

Things that I have been going through and refactoring:

react-router Link doesn't exist, need to change to next's Link which a) takes href instead of to and b) doesn't render anything. See docs.
react router switch and routes no longer exist. Routing is file-based in the pages folder. I moved AppRoute to components and am using it as a "layout" as suggested in the NextJS docs. See pages/login.tsx
Also note from that page, routes in the pages folder are just a skeleton that has the relevant component from the feature folder. I think this is the best way for now as it keeps as much co-location as possible - may have to modify as we do more server rendering.
There are lots of tests which use and . I put in a next-router-mock library - haven't tested yet but seems it should work. You can set current route with mockRouter.setCurrentUrl and get current with mockRouter.pathname. Can't test if router.back is successful I think...
useHistory -> useRouter. If using outside of event callbacks, you need to check typeof window !== "undefined" because the hook doesn't work on the server.
No route state in nextjs. Will replace with query strings.
REACT_APP_ENV_VAR -> NEXT_PUBLIC_ENV_VAR
image imports are no longer urls, but objects with src, width, height and blurDataUrl

darrenvong · 2021-11-06T15:10:44Z

Also note from that page, routes in the pages folder are just a skeleton that has the relevant component from the feature folder. I think this is the best way for now as it keeps as much co-location as possible - may have to modify as we do more server rendering.

Is the plan to convert the react-snap pages to server-side render, then leave the rest of the app client-side rendered as before and change to server rendering as needed for this first step? Otherwise it will be a lot of code changes...

lucaslcode · 2021-11-06T16:24:07Z

Nothing will be server-rendered in this PR (but everything will be "snapped" ie. static export. Don't think you can do it halfway)

darrenvong · 2021-11-06T16:52:52Z

but everything will be "snapped" ie. static export.

How would that work for pages that require the user to be logged in first? Will it be static HTML with the loader, then React Query fetches data client-side once the app is hydrated?

lucaslcode · 2021-11-06T17:33:16Z

Yes

lucaslcode · 2021-11-14T16:41:53Z

@darrenvong this might be a good point to have a look and make some comments. Only login and dashboard are working, but I also moved across (and fixed type errors of) all the related dependencies (which are a lot!) and also components. I also managed to get all the tests for those files working locally, which took a lot of time replacing MemoryRouter!

darrenvong

Damn this is gonna be a big refactor 😅 not really sure how far in I am with the screwed up diffs (probably 40-50%) so will have to continue looking when I have more time.

What's the plan from this? Are we actually thinking of porting the whole app all in one go once the bumps and issues are ironed out?

How will this work for ops/CI as well? I guess instead of serving files from CloudFront in a S3 bucket, we'd be running this as a dockerised app instead and have nginx route to this... somehow? Curious as to what extra complexity it adds on that front as it looks simple for local development, but might not be the case for ops 😅

app/web/utils/date.ts

app/web/tsconfig.json

darrenvong · 2021-11-17T00:57:42Z

app/web/test/setupTests.ts

+// It needs to be in the form dynamic(() => import("components/MarkdownNoSSR"))
+// This is hacky. Really we need to just ditch any non-ssr components
+/// TODO: Get an SSR-friendly markdown editor
+jest.mock("next/dynamic", () => ({


What happens if you don't mock this? Is this a somewhat known hack used by people to set up tests? 😅

If you don't mock it, it just renders nothing for the component. "Well-known" I'm not sure, but it came up in some googling. Really I want to get rid of it asap by removing anything that doesn't support ssr.

app/web/routes.ts

app/web/.eslintrc.json

app/web/components/Navigation/NavButton.tsx

app/web/features/communities/EditCommunityInfoPage.tsx

app/web/features/communities/CommunityPage/PlaceCard.tsx

app/web/features/communities/discussions/DiscussionsSection.tsx

app/web/features/communities/events/CreateEventPage.tsx

darrenvong

More comments/replies. Will have to continue looking when I get more time again

app/web/components/AppRoute.tsx

app/web/features/communities/CommunityPage/PlaceCard.tsx

app/web/features/communities/EditCommunityInfoPage.tsx

app/web/features/auth/AuthProvider.tsx

app/web/features/auth/login/Login.tsx

app/web/features/communities/CommunityBase.tsx

app/web/features/communities/CommunityPage/CommunityPage.tsx

app/web/features/communities/events/EventCard.tsx

darrenvong · 2021-12-07T00:17:00Z

How you feel about setting up another environment and push this under another subdomain (beta.couchershq.org or something) if we are planning to a full migration/refactor and eventually go across to NextJS with this? I think it'll be good to get a few of our regulars on Slack to do a bug bash first before fully replacing the client-side SPA we have right now, just so that the main bugs are mostly caught/ironed out, as I imagine it'll be quite hard to catch everything even at code review given how big of a job this is.

lucaslcode · 2021-12-08T14:16:58Z

Good idea!

vercel · 2021-12-20T16:29:17Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/couchers-org/couchers/FMctHu9jrjQciVpin35GHhAN9gwn
✅ Preview: https://couchers-git-web-refactornextjs-couchers-org.vercel.app

lucaslcode · 2021-12-20T16:36:11Z

@darrenvong this is kind of done. Some things aren't working in storybook and I need to figure out how to generate the protos on vercel but it seems to be working in general

darrenvong · 2021-12-21T00:02:20Z

How do you see it in the Vercel preview? Or is that not fully working yet and only works locally?

darrenvong · 2021-12-21T00:06:41Z

Also I'll try my best to review, but probably stick to more high level testing/poking around... I'm not sure it's ~~even reviewable from the web UI without getting a big lag with so many files changed/moved 🙈~~ actually seems to be alright once I give it a few mins to load, let's see

darrenvong

Mostly questions, but looks good from what I've gone through so far. Will continue looking when I have more time

app/web/.eslintrc.json

darrenvong · 2021-12-21T00:30:34Z

app/web/components/CookieBanner.tsx

@@ -31,6 +32,9 @@ const useStyles = makeStyles((theme) => ({
 export default function CookieBanner() {
  const classes = useStyles();
  const [hasSeen, setHasSeen] = usePersistedState("hasSeenCookieBanner", false);
+  // since we are using localStorage, make sure don't render unless mounted
+  // or there will be hydration mismatches
+  const isMounted = useIsMounted().current;


With this change recently in React - facebook/react#22114 - I thought this hook might be a bit hacky (and maybe still is, only time will tell how compatible this will be with the newer React 18 concurrent patterns) but interesting to see it become useful again in the context of SSR

I personally don't like the fact it needs .current, I tripped up by omitting that and then obviously isMounted was always true.

Could probably change the useIsMounted hook to return .current within the hook result, but then I think there'll be a few places that use the useSafeState hook will need updating. Based on the linked issue in the React repo, I think the useSafeState should probably be removed and swapped to a normal useState instead, as they acknowledged that warning is mostly a false positive if they have remove it.

darrenvong · 2021-12-21T00:34:04Z

app/web/components/Datepicker.test.tsx


    await waitFor(() => {
-      expect(date?.format().split("T")[0]).toEqual("2021-03-20");
+      expect(date?.format().split("T")[0]).toBe("2021-03-20");


I thought toEqual and toBe works the same when it's a primitive value like strings 🤔

I can't remember if this was necessary or was just left behind from something else. Maybe the latter

darrenvong · 2021-12-21T00:45:36Z

app/web/components/Footer/Footer.tsx

+  containerPadding: {
+    maxWidth: theme.breakpoints.values.md,
+    paddingInlineStart: theme.spacing(4),
+    paddingInlineEnd: theme.spacing(4),


How do these change the look/layout of things with what's added in line 112 compared to before?

It doesn't spread out as much at large breakpoints, but I think it looks good and the previous way doesn't work with hydration. If we want it to look closer to before, we could add media queries here.

app/web/components/MarkdownNoSSR.tsx

darrenvong · 2021-12-21T01:10:06Z

app/web/features/auth/AuthProvider.tsx

@@ -20,26 +20,26 @@ function useAppContext<T>(context: Context<T | null>) {
 export default function AuthProvider({ children }: { children: ReactNode }) {
  const store = useAuthStore();

-  const push = useHistory().push;
+  const router = useRouter();


Do you happen to know if router is memoised/stable? Otherwise, wonder if it defeats the point of using the dependency array check in the useEffect.

You're right it's not: vercel/next.js#18127

Actually, isn't this right? Because if it's not stable, we don't want to accidentally call the "old" router.push?

Yeah, but then we run into that infinite loop issue I think you faced in that other useEffect in some cases so seems like a bug to me? It's unclear what bad things would happen if we call the "old" router.push... I'd assume nothing since it doesn't have router state like react-router? 🤷

Added a workaround taken from that bug thread which allows to use the latest router.push but doesn't trigger useEffect. I also found the t dependency in useAuthStore was causing that useEffect to fire too often too, so fixed that.

lucaslcode · 2021-12-21T02:43:56Z

"successfully deployed" but:

I had to use protos from gitlab develop branch artifact - couldn't get docker working in vercel and could use branch names with / @aapeliv
it's trying to target production api
it's getting cors errors

Will fix the last two later.

app/web/.storybook/i18n.ts

darrenvong · 2021-12-22T00:03:23Z

app/web/features/auth/AuthProvider.tsx

@@ -20,26 +20,26 @@ function useAppContext<T>(context: Context<T | null>) {
 export default function AuthProvider({ children }: { children: ReactNode }) {
  const store = useAuthStore();

-  const push = useHistory().push;
+  const router = useRouter();


Yeah, but then we run into that infinite loop issue I think you faced in that other useEffect in some cases so seems like a bug to me? It's unclear what bad things would happen if we call the "old" router.push... I'd assume nothing since it doesn't have router state like react-router? 🤷

darrenvong

Don't think there was anything major and so mostly questions here and there having gone through the boring stuff. But boring is good, as that means we kept most of the React code fairly intact, so nice job on that!

Only looked through the pages / routing stuff on an ad-hoc (as I mostly went from top to bottom to stop GitHub crapping out on this large diff), but I noticed now the routing logic has moved there instead of some of the Switch stuff we had with React Router. Might it be worth having unit test for that logic? Or is it gonna be a pain since it's bundled within Next specific components now?

darrenvong · 2021-12-22T00:04:57Z

app/web/components/Datepicker.test.tsx

@@ -26,7 +26,7 @@ const Form = ({ setDate }: { setDate: (date: Dayjs) => void }) => {
  );
 };

-describe("DatePicker", () => {
+describe.skip("DatePicker", () => {


Is this meant to be skipped still? Does that mean the DatePicker doesn't work well when server rendered?

I could not for the life of me get this to work - as is currently, it hangs forever. I think it's to do with timezone mock and mocking the Date implementation, but I never got it working.

Maybe we should follow your advice and ditch this anyway for native pickers...

Ah yeah this datetime mock seems pretty buggy and I never got it working outside of these tests... well I guess it just doesn't work if they start failing here too.

darrenvong · 2021-12-22T00:06:50Z

app/web/dl-protos.sh

@@ -0,0 +1,3 @@
+curl -L https://gitlab.com/couchers/couchers/-/jobs/artifacts/develop/download\?job\=protos -o /tmp/protos.zip


Is this only for the preview environment? Or will we need it for production too?

Yeah still figuring this bit out

darrenvong · 2021-12-22T00:10:21Z

app/web/features/auth/signup/Signup.tsx

@@ -178,13 +179,15 @@ export default function Signup() {
          authActions.authError(
            isGrpcError(err) ? err.message : t("global:fatal_error_message")
          );
-          history.push(signupRoute);
+          router.push(signupRoute);


Ahh this was what I'm trying to refer to in AuthProvider - I'm guessing since we ignored router as a dependency here and it's still fine (?), maybe it doesn't matter if we call the "old" router.push?

This case was for the next-router-mock router, not the real one. But see my other comment

Shouldn't we be using the workaround here then, rather than leaving it out of the dependency array?

darrenvong · 2021-12-22T00:32:46Z

app/web/features/communities/PagePage.tsx

 import { service } from "service";
 import isGrpcError from "utils/isGrpcError";

-export default function PagePage({ pageType }: { pageType: PageType }) {
+export default function PagePage({


Lol I hope we rename this "model" in some way so we don't have this PagePage confusion...

darrenvong · 2021-12-22T00:42:10Z

app/web/features/communities/discussions/DiscussionsListPage.tsx

@@ -45,7 +44,7 @@ export default function DiscussionsListPage({
    ...useDiscussionsListStyles(),
    ...useStyles(),
  };
-  const hash = useLocation().hash;
+  const hash = typeof window !== "undefined" ? window.location.hash : "";


Does it not matter this defaults to "" since it will be behind a loading spinner when rendered server side? How does it deal with the change of receiving #new once it's at client side?

Yes, the server just renders the header, spinner, and footer; and initially, so does the client.

darrenvong · 2021-12-22T01:36:13Z

app/web/features/search/FilterDialog.tsx

+  const validateHasLocation = (
+    data: string | number | string[] | number[] | { value: null | unknown }
+  ) => {
+    if (!data) return true;


I guess this also covers data === "" from before since "" is falsey?

darrenvong · 2021-12-22T01:36:48Z

app/web/features/search/LocationAutocomplete/LocationAutocomplete.stories.tsx

@@ -29,8 +29,8 @@ const Template: Story<any> = (args) => {
        params={urlParams.current}
      />
      <p>
-        Pressing enter in the field shouldn't perform a submit action, but the
-        button should.
+        Pressing enter in the field shouldn&apos;t perform a submit action, but


What happens on the server side without converting the character to an escape sequence?

Probably nothing but there was a new lint rule complaining. Maybe I should just disable it.

I guess it doesn't matter that much as we should be moving these messages into JSON files too as part of the i18n work, so probably fine to leave it for now

darrenvong · 2021-12-22T01:42:20Z

app/web/next-i18next.config.js

+  i18n: {
+    defaultNS: "global",
+    defaultLocale: "en",
+    locales: ["en", "fr"],


Not sure if we should have "fr" in here until we have a decent amount of the app translated

That was mostly for me to test :P

app/web/package.json

darrenvong · 2021-12-22T01:49:11Z

app/web/package.json

    "@types/css-mediaquery": "^0.1.1",
    "@types/geojson": "^7946.0.8",
    "@types/google-protobuf": "^3.15.5",
    "@types/jest": "^26.0.24",
-    "@types/node": "^14.18.0",
+    "@types/node": "16.11.6",


Have we upgraded to Node 16 on Vercel?

Also, loosely related, do we not need a Dockerfile for this anymore?

No, not sure why I changed that

lucaslcode · 2022-01-06T15:33:19Z

I am thinking about just ignoring that warning about the tabs. I think the behaviour is correct! What do you think?

darrenvong · 2022-01-06T19:45:29Z

I think the behaviour is correct!

Yeah, otherwise I think a random white space will get highlighted if we add the empty string to the list of possible tabs, which would probably look more weird. I'm not sure anything bad would happen apart from no tabs get highlighted, which seems fine to me.

Only thing I'm less sure of is having the "Messages" title displayed even for host requests, as that might be a bit confusing as those aren't just messages? But maybe it's not worth nitpicking over in this refactor if we are planning to re-do requests separately at some point anyway

darrenvong

Should be the last thing hopefully!

darrenvong · 2022-01-06T21:53:53Z

app/web/features/profile/view/leaveReference/LeaveReferencePage.tsx

+                <ReferenceForm
+                  referenceType={referenceType}
+                  userId={userId}
+                  step={step}
+                />


Suggested change

<ReferenceForm

referenceType={referenceType}

userId={userId}

step={step}

/>

<ReferenceForm

hostRequestId={hostRequestId}

referenceType={referenceType}

userId={userId}

step={step}

/>

Otherwise host/surfer references are still broken

darrenvong · 2022-01-06T22:01:07Z

Have you come across this "unhandled runtime error" whenever you're using the Toast UI editor? Initially I thought this was caused by React when seeing a bunch of similar errors on Sentry, but now the stacktrace here quite clearly points to the editor, so that seems to be the actual issue...

darrenvong · 2022-01-06T22:53:28Z

Another thing - Next seems to put things in a .next folder rather than build, so not sure if the existing Sentry integration would work anymore. Looking at their setup guide, it looks quite different.

It might be worth running through their config wizard that supposedly set up a few things automatically? I think we'll miss some routing errors otherwise with how we set up at the moment.

app/web/sentry.js

darrenvong

Let's ship it and see how it goes 🚀

NextJS

lucaslcode added the 1.topic frontend label Oct 30, 2021

lucaslcode force-pushed the web/refactor/nextjs branch from 2e6da02 to 436a162 Compare November 14, 2021 16:11

darrenvong requested changes Nov 17, 2021

View reviewed changes

darrenvong reviewed Nov 24, 2021

View reviewed changes

lucaslcode force-pushed the web/refactor/nextjs branch from 675e9a6 to fb8edad Compare December 1, 2021 16:33

vercel bot temporarily deployed to Preview December 20, 2021 16:29 Inactive

vercel bot temporarily deployed to Preview December 20, 2021 16:53 Inactive

vercel bot temporarily deployed to Preview December 20, 2021 16:58 Inactive

vercel bot temporarily deployed to Preview December 20, 2021 17:01 Inactive

vercel bot temporarily deployed to Preview December 20, 2021 17:06 Inactive

vercel bot temporarily deployed to Preview December 20, 2021 17:14 Inactive

darrenvong reviewed Dec 21, 2021

View reviewed changes

vercel bot temporarily deployed to Preview December 21, 2021 02:14 Inactive

vercel bot temporarily deployed to Preview December 21, 2021 02:24 Inactive

vercel bot temporarily deployed to Preview December 21, 2021 02:35 Inactive

darrenvong reviewed Dec 22, 2021

View reviewed changes

fix reference routing

d35e736

vercel bot temporarily deployed to Preview January 6, 2022 15:31 Inactive

lucaslcode requested a review from darrenvong January 6, 2022 15:33

fix storybook type

b19e953

vercel bot temporarily deployed to Preview January 6, 2022 15:36 Inactive

darrenvong requested changes Jan 6, 2022

View reviewed changes

lucaslcode added 2 commits January 7, 2022 21:02

Fix host reference routing

29bc058

change source maps location

7558f21

vercel bot temporarily deployed to Preview January 7, 2022 13:05 Inactive

lucaslcode requested a review from darrenvong January 7, 2022 13:05

darrenvong reviewed Jan 7, 2022

View reviewed changes

app/web/sentry.js Outdated Show resolved Hide resolved

remove sourcemap upload

339ce03

vercel bot temporarily deployed to Preview January 7, 2022 14:48 Inactive

lucaslcode requested a review from darrenvong January 7, 2022 14:48

darrenvong previously approved these changes Jan 8, 2022

View reviewed changes

lucaslcode added 2 commits January 8, 2022 13:19

Merge remote-tracking branch 'origin/develop' into web/refactor/nextjs

86c5da7

remove sentry from dockerfile

e5c9cad

lucaslcode dismissed darrenvong’s stale review via e5c9cad January 8, 2022 05:21

vercel bot temporarily deployed to Preview January 8, 2022 05:21 Inactive

lucaslcode merged commit a7a03d8 into develop Jan 8, 2022

lucaslcode deleted the web/refactor/nextjs branch January 8, 2022 10:43

aapeliv added release notes: pending Add to stuff that should be included in release notes release notes: done Was mentioned in release notes and removed release notes: pending Add to stuff that should be included in release notes labels Jan 15, 2022

lucaslcode mentioned this pull request Dec 22, 2021

Problem when inserting a hyperlink on mobile using WYSIWYG editor #4164

Open

aapeliv pushed a commit that referenced this pull request Apr 20, 2024

Merge pull request #2146 from Couchers-org/web/refactor/nextjs

9e1a963

NextJS

		@@ -0,0 +1,3 @@
		curl -L https://gitlab.com/couchers/couchers/-/jobs/artifacts/develop/download\?job\=protos -o /tmp/protos.zip

NextJS #2146

NextJS #2146

Conversation

lucaslcode commented Oct 30, 2021

lucaslcode commented Nov 4, 2021 • edited Loading

darrenvong commented Nov 6, 2021

lucaslcode commented Nov 6, 2021

darrenvong commented Nov 6, 2021

lucaslcode commented Nov 6, 2021

lucaslcode commented Nov 14, 2021

darrenvong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darrenvong left a comment

Choose a reason for hiding this comment

darrenvong commented Dec 7, 2021

lucaslcode commented Dec 8, 2021

vercel bot commented Dec 20, 2021 • edited Loading

lucaslcode commented Dec 20, 2021

darrenvong commented Dec 21, 2021

darrenvong commented Dec 21, 2021 • edited Loading

darrenvong 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

lucaslcode Dec 21, 2021 • 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

lucaslcode commented Dec 21, 2021

Choose a reason for hiding this comment

darrenvong 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

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

lucaslcode commented Jan 6, 2022 • edited Loading

darrenvong commented Jan 6, 2022

darrenvong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darrenvong commented Jan 6, 2022 • edited Loading

darrenvong commented Jan 6, 2022

darrenvong left a comment

Choose a reason for hiding this comment

lucaslcode commented Nov 4, 2021 •

edited

Loading

vercel bot commented Dec 20, 2021 •

edited

Loading

darrenvong commented Dec 21, 2021 •

edited

Loading

lucaslcode Dec 21, 2021 •

edited

Loading

lucaslcode commented Jan 6, 2022 •

edited

Loading

darrenvong commented Jan 6, 2022 •

edited

Loading