-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(next-app-router) implement an adapter for next-app-router #75
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: MinuKang <minukang5874@gmail.com>
Co-authored-by: MinuKang <minukang5874@gmail.com>
Co-authored-by: MinuKang <minukang5874@gmail.com>
Sorry, I forgot about this PR. Thanks for reviewing the code so carefully @minuukang 🥹🥹 |
Are there any adapters for app-router being prepared other than this implementation? If there is any separate work being prepared, I think it would be okay to close this PR! @minuukang Can you tell me what the progress is for the app router? ++ I have some additional thoughts. toss/suspensive provides a ClientOnly component and ClientOnly Suspense If there are users who want to use use-funnel in the app router, there may be a way to encourage them to install toss/suspensive together. Or, we could encourage users to implement the use-funnel component to operate as a CSR by adding documentation. If we do this, we may not need the app router adapter itself. I'm curious about your thoughts on this part. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I came here after seeing the lack of support for app router while looking at the documentation to use use-funnel.
@@ -0,0 +1,33 @@ | |||
'use client'; | |||
|
|||
import { useFunnel } from '@use-funnel/next-app-router'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import { useFunnel } from '@use-funnel/next-app-router'; | |
import { useFunnel } from '@use-funnel/next'; // for app router | |
import { useFunnel } from '@use-funnel/next/page'; // for page router |
Currently, the recommended router for Next.js is App Router, so please give priority to App Router.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@use-funnel/next is already used for pages router.
If place the funnel for the app router in @use-funnel/next and change the implementation of the pages router to @use-funnel/next/page, it will be a Breaking Change.
A major update is required. I think the maintainer’s opinion is important in this. @minuukang
Or we can access it like next-mdx-remote
They also have similar problems, so they provide packages in this form.
import {} from "next-mdx-remote/rsc" // for app router
import {} from "next-mdx-remote" // for pages router
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at my review, I think it might be easy for XionWCFM to misunderstand. I am a member of Toss, but I am not the maintainer of use-funnel! I apologize for any misunderstanding! @XionWCFM @minuukang @SunYoungKwon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just my personal opinion and has nothing to do with Toss as a whole or use-funnel. The maintainers of use-funnel are @minuukang and @SunYoungKwon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your careful consideration. And I think your opinion is good. 🙂 @manudeli
@XionWCFM sorry for late reply. i think your idea is great.
However, we'll need to revive this PR if we need the results of the use-funnel for a server build. Until then, we can close it. |
I also tested this part. next.js' useSearchParams causes the part corresponding to React's Suspense boundary to render a fallback on the server. And this is similar behavior to ClientOnly in the grand scheme of things. So even though it requires server-side rendering, this App Router Adapter achieves the same results as the Browser + @suspensive/react ClientOnly approach. I think the value of this app router adapter implementation is that it hides knowledge of React's server-side rendering from the user. When using a browser, responsibility for server-side rendering is delegated to the user, and the amount of learning required increases accordingly. Honestly, I'm not sure which value is more important.🥲 I guess it depends on people's opinions. |
Description
Implement an adapter for next-app-router for the issue discussed in #59.
Related Issue : #59
Context
Currently implemented to operate similarly to @use-funnel/browser. Used for history.state. context, history
useSearchParams in next.js requires suspense, you must wrap suspense when using useFunnel.
How Has This Been Tested
I tested locally whether the value was maintained through history.state even when refreshed.
I checked in the local environment whether the funnel steps were changed appropriately even when using the browser's back and forward functions.
Added test case for funnel.render.overlay.
I tested whether pages using useFunnel could be built without problems.
Further Comments
I look forward to app router users being able to use use-funnel too!
If there is anything else that needs correction, please let me know at any time.
thank you
ETC
There is a way to use use-funnel in app router right now.
The value of implementing the next-app-router adapter is that users can use it without the overhead of SSR processing.
However, on the other hand, if only SSR processing is done on the user side, there is no need to implement an app router adapter.
examples
This solution is also available to app router users now that this app-router-adapter has not been released.
If you want to use use-funnel in app-router now, you can follow the solution based on the above two guidelines.
Below is an example that works in the simplest way.
Or can also do this