[MGNLFE-466] Discovery to support for Next.js 13+ App Router Created: 15/May/23 Updated: 13/Oct/23 Resolved: 14/Jul/23 |
|
| Status: | Closed |
| Project: | Magnolia Frontend Helpers |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Improvement | Priority: | Critical |
| Reporter: | Bartosz Staryga | Assignee: | Oanh Thai Hoang |
| Resolution: | Done | Votes: | 2 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | 4d 1.5h | ||
| Original Estimate: | Not Specified | ||
| Attachments: |
|
||||||||
| Issue Links: |
|
||||||||
| Template: |
|
||||||||
| Acceptance criteria: |
Empty
|
||||||||
| Task DoD: |
[ ]*
Doc/release notes changes? Comment present?
[ ]*
Downstream builds green?
[ ]*
Solution information and context easily available?
[ ]*
Tests
[ ]*
FixVersion filled and not yet released
[ ] 
Architecture Decision Record (ADR)
|
||||||||
| Date of First Response: | |||||||||
| Epic Link: | SPA Editor Backlog | ||||||||
| Team: | |||||||||
| Work Started: | |||||||||
| Description |
|
This has been the Discovery Ticket. The feaature will be implemented with this epic: https://jira.magnolia-cms.com/browse/MGNLFE-558
Next.js 13 introduced new routing mechanism called: App Router It is recommended way of routing for new apps, and old should migrate as per: https://nextjs.org/docs/app It also introduced the idea of Server and Server Components, to explicitly split between code rendered on server and on client side. @magnolia/react-editor is using React context to share data between components. TypeError: createContext only works in Client Components. Add the "use client" directive at the top of the file to use it. Read more: https://nextjs.org/docs/messages/context-in-server-component Using `use client` makes the fetching of page data happen on client side = opposite what we want from Next.js We should refactor React lib to stop using context for sharing data across components. Ideally using vanilla JS that would avoid conflicting with any changes in the future. |
| Comments |
| Comment by Bartosz Staryga [ 16/May/23 ] |
|
Now all libs, React/Vue/Angular use some sort of framework specific context approach. The vanilla solution to share config+template annotations across all the components would allow to make all the libs bit more framework agnostic and more similar in use. Re need to share template annotations might be wort to revisit the idea: https://jira.magnolia-cms.com/browse/MGNLREST-625
|
| Comment by Oanh Thai Hoang [ 02/Jun/23 ] |
|
Output discovery: For using app router in next js 13. Developer need to change code in Next Js to detect in Editor via url param instead of relying on react editor.
Here is my proposal of moving from Pages Router to App Router that using next js 13.4.4 and react 18.2
There are some main points I can summarize.
Disadvantages: always fetching template annotation in server component is not optimal. Because there is no way to detect url in server component as well.
After discussing with canh.nguyen , he and I think provide a flag for spa to render greenbar should be better instead detecting mgnlPreview=false in url Provider a flag to me it would increase user experience. In production, we don't need to fetch annotation and don't need to show greenbar so just disable a flag.
Honestly, I'm not good at front end development so I still need feedback from bstaryga , or rsiska
|
| Comment by Bartosz Staryga [ 07/Jun/23 ] |
|
Hey oanh.thai For using app router in next js 13. Developer need to change code in Next Js to detect in Editor via url param instead of relying on react editor. That is not gonna help here
We place our EditablePage in server component, as we wanna render whole page on server side. Editable page is using React Context to share some data between child components: The app router will throw an error cause of this bits: https://git.magnolia-cms.com/projects/MODULES/repos/frontend-helpers/browse/packages/react-editor/src/util/EditorContext.js#4 The issue is that with App router Context API only works in Client Components (components rendered in browser). Here is the Next.js error page: https://nextjs.org/docs/messages/context-in-server-component We can not render pages on client side, as it would counter the whole idea of using Next.js.
The way forward I see is to replace context with vanilla solution to share config+template annotations across all the components (as per comment above |
| Comment by Oanh Thai Hoang [ 08/Jun/23 ] |
|
Thanks bstaryga for your feedback. I will investigate again and try your suggestion |
| Comment by Oanh Thai Hoang [ 16/Jun/23 ] |
|
Discovery status update: From what I got so far (with above 2 draft PRs). Our react-editor can work with new App router of next js 13. But still need to place EditablePage, EditableArea under "use client" because they are interactivity and still use State and browser API and so on Here is suggestion from next-js 13 when to use server and client component.
To me, get rid of using "use client" for EditablePage in Next js 13 is impossible. Additionally, Next Js 13 propose us suggestion when to use server and client component. From next js document about Client component. Client Components enable you to add client-side interactivity to your application. In Next.js, they are pre-rendered on the server and hydrated on the client. You can think of Client Components as how components in the Pages Router have always worked. From what I understand, use clients is acceptable from next js 13. bstaryga. WDUT? |
| Comment by Bartosz Staryga [ 16/Jun/23 ] |
|
Heyo, we do need to get rid of client components aka use client as we must render whole page on server side, we can not hydrate the page content on client |
| Comment by Christopher Zimmermann [ 16/Jun/23 ] |
|
Yeah anoblet , would be great if you could take a look. |
| Comment by Christopher Zimmermann [ 16/Jun/23 ] |
|
This is kind of annoying...
I dont think we want every component to make a REST request! |
| Comment by Christopher Zimmermann [ 16/Jun/23 ] |
|
Could this "global singleton" work?
But not for a connection - for some actual downloaded data? |
| Comment by Christopher Zimmermann [ 16/Jun/23 ] |
|
Might part of the solution be two different kinds of react-editor components? One set that is used in the editor experience with "use client", and another set only used for SSR/SSG rendering which are the true server components? |
| Comment by Adrien Noblet [ 19/Jun/23 ] |
|
tl;dr As library maintainer, we unfortunately can't ship our code with 'use client' or force our customers to use it side-wide because 1. forces the page to render as client components (see below) and 2. negates the server components benefits altogether. In order to fully support server components requires a meaningful rewrite effort (see below). — I started with Oanh work, and it was incredibly helpful to get into the issue! (here and here Why no `use client`? Changes required:
|
| Comment by Christopher Zimmermann [ 19/Jun/23 ] |
|
oanh.thai Do you think you are able to proceed with the above input? Or of course continue the conversation as needed - or bring up anything that you think requires PM decision. |
| Comment by Oanh Thai Hoang [ 20/Jun/23 ] |
|
Thank anoblet for your changed required suggestion. It's really help me. Item 1 have been done in PR's that you have tried.
Hi czimmermann: if I apply changed required from anoblet, the most complex part is below one
It will be a big refactor for react-editor for sure. I doubt that we couldn't remove `use client` completely from react-editor. We still need some place that use client code (Ex: still need PageEditorBridge.init in EditablePage..).
And regarding your below suggestion
Two different kinds of react-editor components like your idea above is also my choice. Actually, I also think about exporting a new react-editor-v2 with new react components set and only working from react 18 and above.
|
| Comment by Adrien Noblet [ 20/Jun/23 ] |
|
You're absolutely correct about the interactivity needed inside EditableComment. I do realise that what I'm suggesting is a significant effort! Couple additional thoughts:
|
| Comment by Oanh Thai Hoang [ 28/Jun/23 ] |
|
Hi czimmermann . Here are update of discovery output: Look at proposal PR for detail after I apply changed suggestion from anoblet in above thread comments
For react-editor project:
Create a different set of EditablePage, EditableArea, EditableComponent, EditableComment in next JS due to:
Unfortunately, my discovery hasn't finished yet because I still stuck in how to bundle a JS react library that can support "use client" (moving EditablePage, EditableArea, EditableComponent, EditableComment to a new react project) and export them to js file so next js can import component easily.
As I explain above,I still decide to keep EditableComment and RegisterEvents to use "use client". And Next JS does not support well to export component from shared library that include 'use client'
From next js document: "It's worth noting some bundlers might strip out "use client" directives.
Here are some below reference link that I have follow and try:
|
| Comment by Christopher Zimmermann [ 29/Jun/23 ] |
|
oanh.thai Sounds complicated. And sounds like there are some ecosystem blockers (or potential blockers), like the bundlers. Sadly I am not deep enough in the topic to understand much of the above right now. Especially that it may require changes to PageEditorBridge. Still it seems like we are making some progress - and it seem like the move where possible to vanilla JS is a good path. I can say that we can certainly consider making big changes to the library and go for a 1.5 or 2.0 version of react-editor that is not backwards compatible. I could see releasing an alpha version so that customers and partners can try it and give feedback. One question I have - would the new version support both Next page router and app router or only app router? I think it would be good to get input from you too bstaryga and rsiska . (I guess you've seen tha above anoblet ) |
| Comment by Oanh Thai Hoang [ 03/Jul/23 ] |
|
Hi czimmermann .
It should support as long as Next JS 13 can work in both page and app router.
At the end. I come up with this PR. My problem about bundling client component has been gone after I tried to write those client components with typescript. As you agree that we can introduce a new react-editor so I don't have any problem so far.
To summary a bit my proposal in PR. We have to create a new react-editor-2 and don't need to change template-annotations module. Look at my structure below
react-wrap-client-component-typescript that contains 2 client component ('use client')
And last one is nextjs13-app-router-ssg-minimal that import react-editor-2 only and use app router.
|
| Comment by Adrien Noblet [ 03/Jul/23 ] |
|
This is a significant step forward! As far as I understand: it limits the use of `'use client'` to `WrapperEditableComment` (and its children), so we don't opt-out the customer from server components, only our own code that is used for the editor. Which makes sense, since the comment has interactivity (click, drag, etc).
These are the first few broader/overarching comments but should I review the PR fully and add comments there? |
| Comment by Oanh Thai Hoang [ 03/Jul/23 ] |
|
Thanks anoblet for your comments. All of them are really great. You do not need to review PR carefully because it's just a way to show output of discovery process. Until ticket add to our sprint and we have a a ready-to-review-PR, then I will add you as reviewer and would love to see your comment later Regarding your feedback above:
--> It is not a breaker because RegisterEvents and WrapperEditableComment do not expose for using public. I have to separate client component to bundle with https://tsup.egoist.dev and https://esbuild.github.io/. I have no glue clearly now but since NEXT 13.4 has just introduced stable app router in 4 May this year so not much bundlers support for this right now except tsub and typescript. Actually I did try with tsub and js, and some another bundlers but NEXT can not recognize react component with "use client"
--> Agree that It needs more clever. I just try a small change for calling in EditablePage because I don't see data is update to date after editing or saving component.
--> Sure, just a hook to add css and vaadin js to register msg events. Without this, green bar (editable comment) can not be green and no interactive (move, drag, drop..)
--> Just a sample demo for next js 13. `WrapperPages` does not need. It can be inside page.js in app router
--> Sure, they will be changed in ready-to-review PR
Thank you a lot with your feed back anoblet |