Survey app router migration

[henrycatalinismith]

Going to drop a quick summary of the survey app router migration thing here before getting started as it's been a long week since this was all discussed and it's at risk of getting forgotten completely unless it's captured in writing now!

#1718 includes the Next.js app router. Once that issue's closed, it'll be possible to begin using the App Router for Zetkin functionality. A clear first use-case for that is the new version of the survey.

Previously the survey's needed people to log in in order to fill it in. It's now become desirable to remove that limitation.

This is also a convenient first step for the overall app router migration. That's because the scaffold() function that all the page router code uses will need to become something quite different for app router code, which means it's nicer shaping new solutions (such as allowing access to a feature like this without requiring login) around the new API instead of the old one.

[henrycatalinismith]

Cool, this was a fun first swing of the axe tonight. It led to 41f6b4c, which I won't even put into a draft PR yet as there's no clear target branch until the work #1636 and #1719 coexist in a single branch.

The need for the new <SurveyForm /> component is as a means of addressing the new requirement to define a boundary between the server and client components. Using the exact same code from src/pages was producing the following error, and the introduction of <SurveyForm /> with its 'use client' directive fixed it.

I've left the POST request handling code completely behind for now. I suspect it's relatively easy to port to a route handler and that we'll be able to throw away that request parsing code too ¹. The interesting problems here are in the Material UI & Emotion part of town. Here's the latest error when you hit http://localhost:3000/o/1/surveys/20 @ 41f6b4c.

Going to hit pause on this for tonight, so here's a little snapshot of the kind of stuff I've been browsing while researching how to fix this one.

• https://www.google.com/search?q=createContext)+is+not+a+function+app+router+emotion
• https://mui.com/base-ui/guides/next-js-app-router/
• Plans to support Next.js 13 - /app directory emotion-js/emotion#2928
• TypeError: (0 , _react.createContext) is not a function on Nextjs using Redux vercel/next.js#49074

Copilot's suggested trying one of those webpack config resolve alias hacks in next.config.js too, which I don't really see why it'd help in this case but I might just give it a try for the sake of completeness next time around anyway. This Next BC break stuff never feels unsolvable. Always just a matter of putting the time in to figure it out and it eventually gives way. Dead sure this will click soon-ish.

Footnotes

1. https://github.com/vercel/next.js/discussions/46483 ↩

[henrycatalinismith]

Updating @emotion/react and @emotion/styled fixed the above. After that, I tried to use useSurvey() to load the survey data in order to render the page.

This produced an EnvContext missing error (first screenshot below). I fixed that by adding <EnvProvider> to the root layout. This itself was a little tricky at first because importing core/store directly in the root layout produces a You have a Server Component that imports next/router. Use next/navigation instead. error (second screenshot below). From there I just added other context providers until the errors stopped and the page rendered. An example of one of those is third in the screenshots below.

EnvContext missing	Server Component that imports next/router	Could not find required 'intl' object

All the above eventually produced 9153a7f. This is really rough work here. Poke around in that diff and you'll see I've hardcoded the intl props to placeholder values for example. I just wanted to punch through to the finish line with this one and kind of try to expose more of the surface area of the problem.

Here's a screenshot of the app router version of the survey page rendering "successfully" using real API data.

Going to hit the brakes again for the night cos this is such a lovely milestone. Going to braindump some of the loose ends first:

• The broken styling seems likely to be because of some missing context provider. Guess in general src/app/layout.tsx is gonna need to match src/pages/_app.tsx pretty closely.
• We don't want two separate createStore() calls like this in the eventual PR, I think. That's one super important detail to sort out.
• And IDK if we actually want this <EnvWrapper> component I've created here in the long run, cos it was really just a quick hack to jump past one of those please put this in a client component errors tonight. Might be necessary to have the component itself, but an improvable name at the very least.

[henrycatalinismith]

Renamed <EnvWrapper /> to <ClientContext /> and added the Material UI-related providers in 988279d. Nice little baby step forwards, didn't expect to get any time for this on a day like today!!

Before	After

[henrycatalinismith]

Today I've dealt with the hardcoded const lang = 'sv';. The resulting commit is e4f2098, which focuses on tweaking getBrowserLanguage to support receiving its input as a string rather than a NextApiRequest. This was necessary because the new way of accessing request headers in the app router is via next/headers rather than via the request object.

If you'd like to read dozens and dozens of frustrated comments begging Vercel to reinstate access to the request object, check out vercel/next.js#42732. Guess they have some kind of grand vision here. Presumably abstracting away the request object like this enables some kind of edge function magic. Whatever. Ours not to reason why. They have an FAQ entry about this too.

Went back and forth a few times about the right way to do this. In the end I just opted for the approach that produced the smallest possible diff. I know it's a bit ugly having a function whose argument type is NextApiRequest | IncomingMessage | string. Happy to change it in any way, and also happy to just let it resolve itself when the app router migration is finished one day and the request object code path can be deleted entirely.

[henrycatalinismith]

Hey cool, big milestone: in 691d178 I've commented out the user survey signature option. This was something we discussed earlier on as a helpful migration shortcut made possible by the fact that the likely first campaign this page would be used for doesn't depend on this functionality. Doing so has gotten this page to an error-free state!

Before	After

This means that the next step here will be the route handler for the POST request!

[henrycatalinismith]

Okay, feac52e has one possible approach to handling survey form submissions via the app router. Unfortunately it breaks the noscript version of the form. I'm not happy about this at all, but it seems to be the direction Vercel's taking Next.js whether we like it or not.

In the request object discussion I linked earlier, a couple of the threads cover this very topic.

• Access Request Object in New App Directory Page Route vercel/next.js#42732 (comment)
• Access Request Object in New App Directory Page Route vercel/next.js#42732 (comment)

In that second one, one of the Next.js team claims progressive enhancement is still possible with the app router if you also adopt server actions. The community member then attempted to explore that suggestion, without success. And if you dig a layer deeper by clicking on some of the links in the docs for this functionality, you notice it's using stuff like useFormState whose docs say things like

My intuition's telling me server actions are a bit too bleeding edge for Zetkin's purposes right now.

So that leaves options like feac52e, where we intercept the form submit event and hand it off to an /api/… route. I've tried to implement this as neatly as possible, removing all the dead formData code paths and setting up role="alert" on the error message so as not to introduce any new accessibility issues into the bargain.

[henrycatalinismith]

Commit 3989273 contains a working server action. The trick was to move the server action definition into its own file.

Due to the function no longer being defined in a scope where orgId and surveyId are available, it was also necessary to pass those back around to the server action as hidden input fields. It feels like this is a rough edge that might be possible to tidy up somehow, but not a showstopper.

The Next.js docs say you can inline server actions as long as you put "use server" at the top of them but when I do that, it errors like this. The change in the screenshot below inlines the server action in the exact same way as the Next.js docs but produces the error message on the right.

[henrycatalinismith]

Closing this now as today's hackathon progress enables us to consolidate these efforts into #1756 rather than continuing with multiple parallel threads of development.