-
Notifications
You must be signed in to change notification settings - Fork 24
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
[Bug]:@use-funnel/browser is not being processed on the server side. #59
Comments
Wow! I thought when using 'use client' directive, should I think we should add const isServer = typeof window === 'undefined';
const [location, setLocation] = useState(() => ({
search: isServer ? '' : window.location.search,
}));
const [state, setState] = useState(() => ({
...isServer ? {} : window.history.state,
})); This can be not rerender at first render compared by useEffect solution. |
That solution creates a hydration error in non-initial cases. This is related to the The server always returns the initial value, but the render actually attempted by the client produces different results depending on the window object, so the results of server rendering and client rendering are different. Therefore, to avoid hydration errors, we had to handle side effects in However, if you tolerate hydration errors, this approach was one of the solutions to avoid UI flickering. What do you think? |
oh i doesn't understand about next.js app router build and 'use client'. You solution is right. it should be inside in effect by only using client. |
I will use |
I also thought of that method and tried using layoutEffect, but the flickering was still there. It is difficult to explain the reasons for this, but the phenomenon can be identified. Test it in your app router using the simple code snippet below: export default function Home() {
const state = useExample()
return (
<div>
{state}
</div>
);
}
const useExample = () => {
const [state,setState] = useState('default state')
useLayoutEffect(() => {
setState('layout state')
} , [])
return state
} If you run this code in the app-router environment, the layout state will blink after the default state. When I tested it, useFunnel was the same. The only reliable way to eliminate flickering that I have found so far is using Server-side processing seems difficult. What do you think? |
Thats right. it should be add documention with using |
Personally, I think it would be a good idea to add documentation until the app-router-adapter is ready. Even if you do not change the current implementation of @use-funnel/browser, if you import a component that uses useFunnel through dynamic on the user side, you will be free from flickering problems and server-side build problems. Perhaps the app router avoids this flickering problem by triggering Suspense until useSearchParams is ready. So I haven't tried it yet, but creating an app router adapter seems like a faster solution than fixing @use-funnel/browser. I would like to see if there is anything I can do to solve this problem. If you have any plans to create app-router-adapter, I would like to participate. |
If we want to add new packages, we need to be able to add an array of funnel states to Like: https://github.com/toss/use-funnel/blob/main/packages/browser/src/index.ts#L36-L40 And you can test add adapter package, follow this link |
Unfortunately, the suggestion in #63 only works with pages routers. This is because the app router and pages router use different router instances. If using history.state is so important we should probably create our own. An implementation like 'useHistory' that triggers suspense instead of producing a blink without causing a hydration missmatch. I've tried several simple methods, but I haven't found a way to implement all of these requirements yet. next.js's useSearchParams seemed to take the approach of checking whether the above requirements are a server by checking whether a specific instance within next.js exists only on the server, and then skipping pre-rendering by raising an error if it is a server. next.js useSearchParams implementation | bailout-to-client-rendering.ts adopt this approach, i can skip rendering on the server and always access the window object on the client. Instead, there was no way to ignore server errors. Maybe I'm just stuck with the next.js implementation and not being imaginative. Do you have any ideas? |
After discussing the refresh implementation of Currently, We are looking to improve this to store the current step and context in a query string, and only store the "old" history in After some more testing, I'm going to merge that PR. Feedback on that PR is also welcome! |
@use-funnel/next I understand what trick it was implemented with. So it was different from what you wanted. I think storing the context in the query string is a really good idea. I'm looking forward to the next version of @use-funnel |
I'm currently writing code for an app router adapter. It seems like a good idea to keep only previous records in history.state! I haven't tried it yet, but I think this method might be of great help in creating an app router adapter. I haven't yet solved the issue of flickering caused by history.state, but I'm thinking of trying a method of moving only the current context to the query string. I'll be back with the code thank you! |
Package Scope
@use-funnel/browser
Bug description
I think this is a bug.
@use-funnel's docs recommend using @use-funnel/browser when using the next.js approuter, but
In fact, if you use @use-funnel/browser in the app router, the project cannot be built because server-side processing is not performed properly.
This issue can also be found in @use-funnel/browser@0.0.5 version.
The reason this problem occurs is that using the 'use client' directive when using the next.js app router does not mean that the file will never be evaluated on the server side.
In fact, next.js also analyzes files containing the use client directive during the build process, and if there is code that accesses the window object, an error occurs.
If we want to pass the use case for app router to @use-funnel/browser, I think we will have to do server-side processing.
Expected behavior
We want error-free builds.
To Reproduce
@use-funnel/browser 0.0.5
next 14.2.13
Possible Solution
I made a few modifications to this code base locally to resolve this issue.
Simply put, the problem of errors occurring during build can be solved by modifying the code so that it does not directly access the window object.
Because this is exactly the code that is causing the problem.
This code raises an error in an environment where the window object does not exist.
So, you can modify it like this:
This code is buildable because it does not access the window object on the server side and only reflects the state through useEffect on the client side, and can also avoid the
Text content does not match server-rendered HTML
error.However, there are user experience issues with this approach.
Since the initial value of the server is first rendered and updated according to the current position through useEffect, if it is not initialStep, initiailStep is shown first and then the UI blinks as the desired step is rendered.
What's even more disappointing is that this approach has the potential to affect use cases that don't use app-router.
So I think there are two options.
Create a new adapter for app-router, place a dependency on next, and create a package that uses hooks provided by next.js, such as useSearchParams.
Modify the @use-funnel/browser package to take server side into consideration.
In my opinion, if there is a way to solve the problem of UI flickering when @use-funnel/browser properly supports the server-side environment while also properly supporting the server-side environment, that would be the best method.
Alternatively, creating an adapter for app-router seems attractive.
Do you have any ideas?
etc.
Or there is another way: Using dynamic in next.js ensures that it will always run only on the client, thus solving UI flickering issues and build issues.
However, this method requires more knowledge from the user.
thank you!
The text was updated successfully, but these errors were encountered: