Skip to content

Conversation

@jonkafton
Copy link
Contributor

@jonkafton jonkafton commented Oct 29, 2024

What are the relevant tickets?

N/A

Description (What does it do?)

Upgrades to Next.js v15.0.1.

  • Server component params and searchParams are now async.
  • The homepage carousels needed a <Suspense> boundary to prevent the useSearchParams() should be wrapped in a suspense boundary build error. The carousels do call useSearchParams() (ResourceCarousel -> ResourceCard -> SignupPopover), but it's not clear why v14 didn't error on this and v15 does.
  • Upgrades @mui/material-nextjs peer dependency.

How can this be tested?

The main workspace should build successfully and run without error.

Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes make sense to me 👍 (except the container change I mentioned).

Annoying that resource cards use useSearchParams (in order to create the href on the link, which we want to preserve the existing search params). And I guess that really needs to be useSearchParams so that the server can render it.

Oof, I was anchor tags just had an attribute to preserve search params =/

One other error

I also noticed this error after running NODE_ENV=production yarn workspace main build and NODE_ENV=production yarn workspace main start:

ReferenceError: DOMParser is not defined
    at 80458 (/Users/cchudzicki/dev/mit-open/frontends/main/.next/server/app/program_letter/[id]/view/page.js:1:5932)
    at Function.t (/Users/cchudzicki/dev/mit-open/frontends/main/.next/server/webpack-runtime.js:1:485)
This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). The promise rejected with the reason:
ReferenceError: DOMParser is not defined
    at 80458 (/Users/cchudzicki/dev/mit-open/frontends/main/.next/server/app/program_letter/[id]/view/page.js:1:5932)
    at Function.t (/Users/cchudzicki/dev/mit-open/frontends/main/.next/server/webpack-runtime.js:1:485)

I believe this is coming from our usage of DOMParser in frontends/ol-ckeditor/src/components/CkeditorDisplay.tsx, which gets used inside ProgramLetterPage.tsx

I think the best fix right now is to make ProgramLetterPage stop using CkeditorDisplay. There's not a good reason to use it since program letters aren't created with CKEditor. (CkeditorDisplay does apply some basic styles; 90% of its CSS does not apply, but a bit of it does, and it does some extra things to handle oembed.)

This error seems not to affect non-program letter pages. It does cause program letter view to 500 error.

return (
<>
<LearningResourceDrawer />
<Container>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving this container broke some stuff. Mistake?

Screenshot 2024-10-31 at 1 21 26 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof, yes.

Comment on lines +19 to +23
"@mui/base": "5.0.0-beta.61",
"@mui/lab": "6.0.0-beta.14",
"@mui/material": "^6.1.6",
"@mui/material-nextjs": "^6.1.6",
"@mui/system": "^6.1.6",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be inclined to do this separately. Seems to have gone smoothly, but a major bump of a significant package.

I'd also be curious the impact on our package size. I think v6 is supposed to be a good bit smaller.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mui/material-nextjs@6.1.6 is needed for Next.js v15 and others were in the same material-ui release (except @mui/base)

@jonkafton
Copy link
Contributor Author

I think the best fix right now is to make ProgramLetterPage stop using CkeditorDisplay. There's not a good reason to use it since program letters aren't created with CKEditor.

That indeed fixes it - surprised that a passing build does not mean that it will start without error. That really means we need to try and run it in CI.

For the editable HTML - would be good to have a rich editor that outputs markdown, if that meets our needs.

Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Two very minor things:

  • Might as well update the package.json to 15.0.2
  • Change "@mui/material-nextjs/v14-appRouter" to "@mui/material-nextjs/v15-appRouter" in `ol-components.
    • Though, as far as i can tell, the v14 and v15 versions are just re-exports of the v13 version. Still seems good, though.

@jonkafton jonkafton merged commit 53411ea into main Nov 1, 2024
11 checks passed
@odlbot odlbot mentioned this pull request Nov 5, 2024
12 tasks
This was referenced Nov 5, 2024
@rhysyngsun rhysyngsun deleted the jk/upgrade-next-15 branch February 7, 2025 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants