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

Functions are passed as Promises in Server Actions #10

Open
iamomiid opened this issue May 9, 2024 · 6 comments
Open

Functions are passed as Promises in Server Actions #10

iamomiid opened this issue May 9, 2024 · 6 comments

Comments

@iamomiid
Copy link

iamomiid commented May 9, 2024

Hi!

If you export server actions with memoize function, any call to any other function is going to be promisifed, too.

For example, let's say we have this file:

"use server";
import { desc, eq } from "drizzle-orm";
import { db } from "../db";
import { Posts } from "../db/schema";
import { memoize } from "nextjs-better-unstable-cache";

export const getPostMetadataById = memoize(
	async (id: number) => {
		return db.query.Posts.findFirst({
			where: eq(Posts.id, id),
			columns: { title: true, description: true },
		});
	},
	{
		revalidateTags: (id: number) => ["posts", "metadata", `post-${id}`],
	},
);

In this case, no errors will be thrown but revalidateTags will be an async function and since we're calling it without await the only tag is going to be all.

I think it makes sense to check if functions are async or not in

revalidateTagsFn(...args) : revalidateTagsFn

and

additionalCacheKeyFn(...args) : additionalCacheKeyFn

I can do the fix myself but I just wanted to make sure that I'm not the only one with this issue.

Versions

  • next: 14.2.3
  • nextjs-better-unstable-cache: 1.1.0
@alfonsusac
Copy link
Owner

Similar with

I dont think you should call memoize in "use server", but i will add a suppressable warning

@alfonsusac
Copy link
Owner

Im not sure why calls to other functions get promisified. Is this a react behavior? This wasnt a problem in 14.1.0

@iamomiid
Copy link
Author

I think it's a fairly new thing because I haven't seen it till recently, too.

I don't know if they're going to keep the current solution or change it in the future but I think it's worth doing something about it (either a warning or making every function a promise 🤷‍♂️)

I dont think you should call memoize in "use server", but i will add a suppressable warning

May I ask what's the reasoning behind it?
The scenario that I'm trying to solve is something like this:
Let's say you have a function to get N latest posts:

"use server";
import { desc, eq } from "drizzle-orm";
import { db } from "../db";
import { Posts } from "../db/schema";
import { memoize } from "nextjs-better-unstable-cache";

export const getLatestPosts = memoize(
	async (limit: number) => {
		return db.query.Posts.findMany({
			columns: { id: true, title: true },
			orderBy: desc(Posts.createdAt),
			limit,
		});
	},
	{
		revalidateTags: (limit: number) => ["posts", `posts-limit-${limit}`],
	}
);

Does it make sense to create a new memoized version of it and provide tags, duration, etc.

If you keep it all in a use server file, you can prevent yourself from repeating the same code over and over again!

@alfonsusac
Copy link
Owner

alfonsusac commented May 16, 2024

May I ask what's the reasoning behind it?

Because data fetching from memoize are sometimes also used in initial page render. Calling server action from page render are okay by itself but its could be a security vulnerability IF its not used as a server action.

All exported functions inside a "use server" file can be assumed to be its own public endpoint. And if you dont use auth guards that means i could theorhetically access your data fetching function. Technically this is just a part of security by obsecurity but its a fair concern to just not allow it, expose it in the first place.

Therefore I'd keep server action and memoized data fetching in a separate file. To create different layers:

Data Access -> db calls, queries, fetch to db, can be wrapped in memoize function
Data Processing -> server components, server actions, that access to "data access" and also most importantly, auth checks.

But if you understand the rules you can just break them like an artist 😉

@ScreamZ
Copy link

ScreamZ commented Jul 25, 2024

I'm not sure to understand why (id: number) => ["posts", "metadata", post-${id}], which is not exported, but defined as an inline callback is considered exported ? Can someone explains this to me ?

I made a big topic about it, because this is a big WHY and risk for me vercel/next.js#68155

@benjick
Copy link

benjick commented Sep 20, 2024

I run this as part of my pipeline:

#!/bin/bash

# Directory to search
search_dir="./apps/nextjs"

# Strings to search for
string1="\"use server\""
string2="memoize"

found=0
ignore_dirs=(-path "$search_dir/.next" -o -path "$search_dir/node_modules")

while IFS= read -r file; do
  if grep -q "$string1" "$file" && grep -q "$string2" "$file"; then
    if [[ $found -eq 0 ]]; then
      printf "\n🔍 found the following files:\n\n"
    fi
    echo $file
    found=1
  fi
done < <(find "$search_dir" -type d \( "${ignore_dirs[@]}" \) -prune -o -type f -print)

if [[ $found -eq 1 ]]; then
  printf "\n🚨 We can't use memoize in server actions. See https://github.com/alfonsusac/nextjs-better-unstable-cache/issues/9 for more information\n"
  exit 1
else
  printf "\n✅ No issues found!\n"
  exit 0
fi

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

4 participants