Skip to content
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

[Feature]: @toss/use-funnel Idea Proposal for supporting App DIrectory(Nextjs13) #322

Open
2 tasks done
nerdchanii opened this issue Aug 7, 2023 · 9 comments
Open
2 tasks done

Comments

@nerdchanii
Copy link

nerdchanii commented Aug 7, 2023

Package Scope

  • Add to an existing package
  • New package

Package name:@toss/use-funnel

Overview

Idea Proposal for Supporting the 'App' Directory in Next.js 13(It is Linked issue #228)

Describe the solution you'd like

I have attempted to make @toss/use-funnel support the Next.js's App directory.

Currently, @toss/use-funnel does not provide full support for Next.js@^13. The reason is that it does not support the App directory.

This is seem to Next.js's App directory supports routing using the AppRouter provided by next/navigation, while the Pages directory uses next/router for routing.

Initially, I thought that to enable support for the App directory of Next.js 13 in @toss/use-funnel, it might be possible to receive the useRouter hook from an external source and return the Router object. Although there were slight differences in the signatures of the push and replace methods between the two Router objects, the existence of these methods seemed to make this approach possible.

During the process of testing this approach, I discovered that the useRouter provided through next/navigation returns an AppRouterInstance that doesn't align adequately with the existing NextRouter object from next/router (notably lacking the query attribute). Furthermore, implementing this approach could lead to significant API changes and potentially impact @toss/use-query-param, which is used by @toss/use-funnel. Additionally, I couldn't find a way to inject a router object into the useRouter called within withState.

As an alternative, I propose creating a custom router package named @toss/useRouter. This approach could potentially allow both @toss/use-query-param and @toss/use-funnel to leverage a custom router, addressing these issues.

Another possible solution is temporarily segregating the implementation of useFunnel to specifically support the App directory.

Additional context

#228
next-router-not-mouted in App Dir

@CodePsy-2001
Copy link

I think it would be better to utilize NextJS's router object like we already do. Creating a custom router is beyond the scope of maintenance for the slash library project.

@nerdchanii
Copy link
Author

nerdchanii commented Aug 14, 2023

Yes, that's correct. There might be challenges similar to the ones you mentioned in creating the custom useRouter. Additionally, keeping the library aligned with changes in NextJS's API could indeed be quite challenging.

So, what would be a recommended approach to support NextJS's app router? Would it be necessary to develop an implementation specifically tailored for app routers?

@CodePsy-2001
Copy link

I think the slash library should provide an implementation for AppRouter (e.g. @toss/use-funnel/appdirectory), or people who like the behavior of this useFunnel custom hook should create their own next-use-funnel library. I think it would be nice to create the next-use-funnel library first, and then provide a later implementation in the slash library.

@nerdchanii
Copy link
Author

I agree as well.
However, the current @toss/use-funnel library not only relies on next/router in Next.js but also has a dependency on @toss/use-query. Furthermore, it seems that @toss/use-query also depends on next/router. In other words, if slash's @toss/use-funnel library intends to support the app router, it appears that @toss/use-query would also need an implementation compatible with the app router.

@CodePsy-2001
Copy link

CodePsy-2001 commented Aug 22, 2023

I looked at the entire code and realized that the dependency on next/router is not that big.

  • The shallow routing API in next/router can be replaced with a web native API.
  • The useQueryParam function in @toss/use-query-param can be replaced with a simple type assertion.

In conclusion, rewriting toss code for Next.js 13 App Directory compatibility shouldn't be too difficult.. I've already started working on that for use by my team.

@CodePsy-2001
Copy link

Well, it seems I was wrong, the easiest way to implement the existing behavior of useFunnel in App Directory was to create a custom client hook that detects the popstate behavior of the window and returns the current searchParams. This was no more productive than creating @toss/router, or contributing code to support the shallow behavior of next/navigation.

@CodePsy-2001
Copy link

Update: By giving up some behavior, an implementation via the next/navigation router is possible without complicated access to browser history. I'll share a build of the library later.

@nerdchanii
Copy link
Author

@CodePsy-2001 wow! I feel exciting your works! I hope see your works ASAP!!

@CodePsy-2001
Copy link

CodePsy-2001 commented Mar 28, 2024

Hello! Sorry for the late update. Please refer to this source code.

npm i next-use-funnel
# or
yarn add next-use-funnel

https://github.com/CodePsy-2001/next-use-funnel
(example: next-use-funnel.vercel.app

My library has 100% test coverage and all interfaces are compatible with @toss/use-funnel.
However, there are some differences in internal behavior to ensure a smooth implementation.

  • Replaced the unimplemented store abstraction with a structure that allows us to inject the desired store for each funnel key via SWRProvider.
  • Separated the unimplemented withState function into an HOC. The HOC can take router as a parameter externally, so you can apply other routers besides the Next.js router.
  • Internally, it is getting the values straight from the query library without useRef. As I've been developing, I've determined that I don't need useRef, and I'd like to ask the toss team for what reason they needed useRef in their withState implementation.

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

No branches or pull requests

2 participants