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

Next.js server action not getting executed #7934

Open
RareSecond opened this issue Aug 21, 2024 · 13 comments · May be fixed by #7970
Open

Next.js server action not getting executed #7934

RareSecond opened this issue Aug 21, 2024 · 13 comments · May be fixed by #7970

Comments

@RareSecond
Copy link

Describe the bug

Please bear with me, because I have no idea how to best start. I've also done my best to narrow this down as much as possible, but I haven't been able to provide a minimal reproducible example.

Setting the stage

We were noticing more and more lately that our (Cypress) tests are flakey so I started to investigate it. I've managed to track it down to a custom hook not resolving with the correct data.

The hook, useCurrentUser, is in essence a very simple hook

import { useQuery } from "@tanstack/react-query";

import { getCurrentUserWithRoles } from "@/actions/users";

export const useCurrentUser = () => {
  const { data: currentUserWithRoles } = useQuery({
    queryFn: async () => {
      return getCurrentUserWithRoles();
    },
    queryKey: ["currentUserWithRoles"],
  });

  if (!currentUserWithRoles) {
    return null;
  }

  return currentUserWithRoles;
};

getCurrentUserWithRoles is also a very simple server action that does some database fetching.

"use server";

import { AuthDAL } from "@/data/auth";

export async function getCurrentUserWithRoles() {
  return await AuthDAL.getCurrentUserWithRoles();
}

However, when running cypress open and running a test that relies on the useCurrentUser returning correct information, the hook sometimes fails to return any data.

What I've tried

Adding logs

If I add logs to the necessary places, I can see where it goes wrong

    queryFn: async () => {
      console.log("About to get roles");
      const res = getCurrentUserWithRoles();
      console.log("res: ", res);

      return res;
    },
export async function getCurrentUserWithRoles() {
  console.log("Inside server action");
  return await AuthDAL.getCurrentUserWithRoles();
}

The first log gets printed, but the other two don't appear. So for some reason, it seems that it never reaches the server action.

Removing React Query

If I remove React Query and just rely on useEffect and useState, there's no flakiness and test passes 100% of the time

export const useCurrentUser = () => {
  const wasCalled = useRef(false);
  const [currentUserWithRoles, setCurrentUserWithRoles] = useState<Awaited<
    ReturnType<typeof getCurrentUserWithRoles>
  > | null>(null);

  useEffect(() => {
    if (!wasCalled.current) {
      wasCalled.current = true;

      getCurrentUserWithRoles().then((res) => {
        setCurrentUserWithRoles(res);
      });
    }
  }, []);

  if (!currentUserWithRoles) {
    return null;
  }

  return currentUserWithRoles;
};

Replacing the server action with an actual API call

I also went ahead and created an actual API call (via route.ts) where I returned hardcoded data. And again, it works 100% of the time.

import { NextResponse } from "next/server";

export async function POST() {
  return NextResponse.json({
    groupRoles: [
      {
        groupIdId: "77fc6325-96ff-449b-91a8-cd0a8c79e019",
        role: "ADMIN",
      },
      {
        groupId: "4b102177-be72-4910-a46d-90060ca7b178",
        role: "ADMIN",
      },
    ],
    firstName: "Fred",
    id: "33d4cde9-c366-4c77-9669-decdf0e0c6ad",
    lastName: "Flinstone",
    appRole: "ADMIN",
  });
}
export const useCurrentUser = () => {
  const { data: currentUserWithRoles } = useQuery({
    queryFn: async () => {
      // return getCurrentUserWithRoles();
      const call = await fetch("/api/v1/test", {
        method: "POST",
      });

      const res = await call.json();

      return res;
    },
    queryKey: ["currentUserWithRoles"],
  });

  if (!currentUserWithRoles) {
    return null;
  }

  return currentUserWithRoles;
};

Replacing Cypress with Playwright

I even rewrote the test in Playwright but to no avail. The tests are still just as flaky, feeling even worse (but that may be my mental state acting up by now).

Some extra insights

However, the switch to Playwright has brought some extra insights that may be helpful

  • If I only enable one of the tree browsers in Playwright (so chromium, firefox and webkit), the tests always pass for chromium and firefox, and fail way less for webkit (10% of the times instead of 70%)
  • Same scenario for setting workers to 1 in Playwright
  • I tried this suggestion to no avail
  • All libraries are at their latest versions

Your minimal, reproducible example

I tried, but can't condense it enough to have it always fail

Steps to reproduce

/

Expected behavior

I expect React Query to actually fire the Next.js server action and return the data

How often does this bug happen?

Often

Screenshots or Videos

No response

Platform

  • OS: MacOS
  • Browser: Multiple

Tanstack Query adapter

react-query

TanStack Query version

v5.52.0

TypeScript version

v5.3.3

Additional context

Please let me know what other information I can provide. I felt like I've been going down the rabbit hole, with no solution in sight..

@RareSecond
Copy link
Author

Hi @neehhaa06

While it shouldn't matter, I've tried your suggestion, but nothing changed.

@quintenbuis
Copy link

quintenbuis commented Aug 23, 2024

Having the exact same issue. With me it happen when I quickly redirect to another page but useQuery started already, leaving it in a state of always saying isLoading and never updating or being able to refetch

@quintenbuis
Copy link

quintenbuis commented Aug 24, 2024

This stripped down version allowed me to continually reproduce this. (have not tested if this actually works with the stripped down code)

The component causing a redirect

const Checkout = (): ReactElement => {
	const { t } = useTranslation();
	const { cart, syncCart } = useCartStore();
	const [loading, setLoading] = useState<boolean>(true);

	useEffect(() => {
		if (cart && cart.lineItems.length === 0) {
			redirect('/cart');
		}

		setLoading(false);
	}, [cart]);

	if (loading) {
		return <></>;
	}

	return (
		<div className="container mb-10">
			<div className="flex flex-col lg:flex-row gap-8 mt-8">
				<PaymentMethods />
			</div>
		</div>
	);
};

The useQuery being used where getPaymentMethods is the server action:

const usePaymentMethods = (): UseQueryResult<PaymentMethod[]> => {
	const { shop } = useShopContext();

	return useQuery<PaymentMethod[]>({
		queryKey: ['paymentMethods', shop],
		queryFn: async () => {
			return await getPaymentMethods();
		},
	});
};

export default usePaymentMethods;

Inside PaymentMethod the useQuery is initiated due to it being rendered into the DOM, but the redirect quickly interrupts that call, causing it to be in a constant loading state.

The workaround for this is to not use server actions, but to create an API proxy yourself and there running the action. Not the most ideal fix ofcourse since it just shouldn't happen.

@adamlewkowicz
Copy link

adamlewkowicz commented Aug 26, 2024

I'm facing a similar issue. Few actions are called, but some are pending in an infinite isPending state, and the server action is never called.
It only runs all actions then on full page refresh.

Is there going to be any fix for this, as this is a major issue?

@TkDodo
Copy link
Collaborator

TkDodo commented Aug 26, 2024

Not really sure how to address this tbh. If the logs are correct and you see "About to get roles", it means the queryFn is executing and we do nothing until the queryFn finishes. If the next thing is calling the server action, then I don't see why this wouldn't happen.

That said, it's not recommended (neither from react-query side nor from nextjs side) to actually use serverACTIONS for something that aren't "actions" (things that have side-effects, we also call them mutations here).

Right now, serverActions run in serial. All of them. One by one. So if you have two unrelated queries, they will be queued up. Maybe the flakiness has to do with how cypress performs tests in parallel and they get queued, I really don't know.

I know that everyone sees the benefit of not having to write API routes because server actions are basically RPC calls but this is not what they are meant for. trpc is still pretty good for this with nextJs.

@RareSecond
Copy link
Author

@TkDodo This is coming straight from the Next.js docs

Server Actions are not limited to <form> and can be invoked from event handlers, useEffect, third-party libraries, and other form elements like <button>.

This isn't a mutation in this case, since we are just fetching the data.

I want to again stress the fact that, when using a simple useEffect and (very high level, I know) recreating the behavior of react-query, everything works as expected.

I know this one's probably a massive bitch to debug, but I do think this is something with react-query that's going wrong. Would you want me to try swr and see if it suffers from the same issue?

@TkDodo
Copy link
Collaborator

TkDodo commented Aug 27, 2024

This is coming straight from the Next.js docs

yes, but it's still an "action". The section in the docs is called Server Actions and Mutations, and it's next to Data Fetching and Caching. Using a server action for data fetching is, again, not recommended by next or by us.

I talked to the nextjs team not too long ago and mid-term, they want to provide something like "server actions" but for fetching. But server actions ain't it.

I want to again stress the fact that, when using a simple useEffect and (very high level, I know) recreating the behavior of react-query, everything works as expected.

I hope it's understandable that my answer here can only be:

I want to again stress the fact that, when using a simple fetch and (very high level, I know) recreating the behavior of server actions, everything works as expected.

Bottom line is I really don't even know how to look into this ...

@bartcheers
Copy link

Right now, serverActions run in serial. All of them. One by one. So if you have two unrelated queries, they will be queued up.

This surprised me. I tested this and it seems that it is incorrect, unless I'm missing something - server actions can run in parallel: See this Codesandbox.

@TkDodo
Copy link
Collaborator

TkDodo commented Aug 27, 2024

@bartcheers your sandbox just calls an async function (that is marked as "use server") in a server component.

To really get a server action, that gets transformed to a POST request under the hood, you need to invoke the action from a client component (e.g. in a button click handler).

@bartcheers
Copy link

Thanks @TkDodo for clarifying. Here's another Codesandbox confirming server actions run in series, not in parallel. Great to know!

@TkDodo
Copy link
Collaborator

TkDodo commented Aug 27, 2024

I think we should just amend the docs that it's not recommended to use server actions with useQuery and that API routes are still the way to go. Would someone want to do that?

bartcheers added a commit to bartcheers/query that referenced this issue Aug 28, 2024
Update the useQuery documentation to clarify that server actions should not be used in `queryFn`.
Server actions are meant for mutations, run serially, and cannot execute in parallel, potentially
leading to unexpected behavior. Recommend using API routes instead for data fetching.

Closes TanStack#7934
@RareSecond
Copy link
Author

While I'm starting to see more why we shouldn't use it for data fetching, I do still think that there's an issue on your part.

I recreated the test in Playwright and ran it 100 times (for 3 different browsers, so 300 tests in total).

This was React Query
image

This was swr
image

We will be making the switch to swr, as that's an easier replacement than making the switch to API routes/tRPC.

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 a pull request may close this issue.

6 participants
@TkDodo @RareSecond @bartcheers @quintenbuis @adamlewkowicz and others