alfonsusac / nextjs-better-unstable-cache

Wrapper function for unstable_cache() from next/cache (Next.js Caching)
https://www.npmjs.com/package/nextjs-better-unstable-cache
72 stars 5 forks source link

Functions are passed as Promises in Server Actions #10

Open iamomiid opened 6 months ago

iamomiid commented 6 months ago

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 https://github.com/alfonsusac/nextjs-better-unstable-cache/blob/e4529adb84f675c103ea65a8969a06a36db4ae20/src/index.ts#L75 and

https://github.com/alfonsusac/nextjs-better-unstable-cache/blob/e4529adb84f675c103ea65a8969a06a36db4ae20/src/index.ts#L70

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

Versions

alfonsusac commented 6 months ago

Similar with

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

alfonsusac commented 6 months ago

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

iamomiid commented 6 months ago

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 commented 6 months ago

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 :wink:

ScreamZ commented 3 months ago

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 https://github.com/vercel/next.js/discussions/68155

benjick commented 1 month ago

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