cloudflare / next-on-pages

CLI to build and develop Next.js apps for Cloudflare Pages
https://www.npmjs.com/package/@cloudflare/next-on-pages
MIT License
1.28k stars 126 forks source link

[🐛 Bug]: Build fails if `getStaticPaths` return an empty array #743

Closed gabrielf closed 1 month ago

gabrielf commented 6 months ago

next-on-pages environment related information

System: Platform: darwin Arch: arm64 Version: Darwin Kernel Version 23.4.0: Fri Mar 15 00:12:49 PDT 2024; root:xnu-10063.101.17~1/RELEASE_ARM64_T6020 CPU: (12) arm64 Apple M2 Pro Memory: 16 GB Shell: /bin/zsh Package Manager Used: npm (9.8.1)

Relevant Packages: @cloudflare/next-on-pages: 1.11.0 vercel: N/A next: 14.2.0

Description

getStaticPaths is used to control what pages to generate at build time for a dynamic route. Returning an empty array means "build no pages" for this route. This can happen when the content on the path varies over time and there may be no content at all or it can be used to hide test-pages in production for example.

Having getStaticPaths return nothing works with npm run build && npm start but fails the next-on-pages build with the following error:

⚡️ Completed `npx vercel build`.

⚡️ ERROR: Failed to produce a Cloudflare Pages build from the project.
⚡️ 
⚡️  The following routes were not configured to run with the Edge Runtime:
⚡️    - /[maybe]
⚡️ 
⚡️  Please make sure that all your non-static routes export the following edge runtime route segment config:
⚡️    export const runtime = 'edge';
⚡️ 
⚡️  You can read more about the Edge Runtime on the Next.js documentation:
⚡️    https://nextjs.org/docs/app/building-your-application/rendering/edge-and-nodejs-runtimes

Adding runtime edge to the route yields another error asking you to change edge to experimental-edge which in turn makes the build succceed but the route fail with a 500 error.

Reproduction

https://github.com/gabrielf/getstaticpaths-empty-return

Pages Deployment Method

None

Pages Deployment ID

No response

Additional Information

No response

Would you like to help?

james-elicx commented 6 months ago

When the paths array is empty, the Vercel CLI generates a Node.js function for the route, and there isn't really a way for us to be able to tell if it's supposed to be safe to ignore... I'm not sure there's much we could do here.

Maybe we could add a CLI option like --ignore-invalid-routes=[maybe] - is that the kind of thing that you'd be after here?

gabrielf commented 6 months ago

I wish I understand the build process better… Building with MAYBE=false npm run build and then running with npm start will give a 404 when navigating to http://localhost:3000/maybe so that is what I'd expect next-on-pages to behave also.

If the behavior of npm run build + npm start is not possible to realize using next-on-pages without introducing a cli option then you can close this issue. I would rather find a workaround in my app than introducing another cli option.

At least this issue exists so other may find it when googling so that makes me happy :-)

dario-piotrowicz commented 4 months ago

@gabrielf as @james-elicx has mentioned unfortunately this is something we inherit from the Vercel cli (and can't really do much about), I personally think that it is a vercel cli bug (or close to), but I don't expect Vercel to be too kin to fix it since it doesn't really cause any issue on their network 😖

I'm sorry for the inconvenience but given the current next-on-pages architecture I don't think there's any way we could improve the situation here 😢

We have this eslint rule to try to help users avoid the various pitfalls of getStaticPaths: https://github.com/cloudflare/next-on-pages/blob/main/packages/eslint-plugin-next-on-pages/docs/rules/no-pages-nodejs-dynamic-ssg.md

But unfortunately the length of the paths array is not something that can be reliably statically analyzed, so unfortunately the eslint rule doesn't help much with this one 😓

dario-piotrowicz commented 4 months ago

I wish I understand the build process better…

The basic idea is that the next-on-pages CLI calls vercel build which generates an output that we follows a specific vercel standard that we know how to deal with. That output contains lambdas and edge functions, we take the edge functions and generate a worker from those. If we find lambdas we error since those are not compabile with the Cloudflare network. Sometimes some specific lambdas we know that are not actually used and that they can be simply ignored, so in certain specific scenarios we can throw them away and still successfully build the worker.

So the issue here is that, when we call vercel build we get a lambda for the [maybe] route, and there is no indication at all that this lambda can be ignored, so what's happening is that we just fail to build the worker.

what James was suggesting was a potential cli option that allows devs to specify additional lambdas that could get ignored (I have thought of this many times, implementing it would be pretty trivial, but DX wise it feels like quite a double edge sword which could likely cause more harm than good so that's why we haven't really implemented it).

Why does `vercel build` generates a lambda for `[maybe]`?

`vercel build` generates lambdas for routes that contain `getStaticPaths` with fallbacks, that's how they implement their SSR/on-demand-SSG. when a route uses `getStaticPaths` and returns an empty `paths` array but with a `false` fallback, this should just not generate anything I guess... but instead of that the `vercel build` command still generates a lambda... I haven't looked to deeply as to why to be honest, my best guess is that they assume that a static route with an empty `paths` and a `false` fallback doesn't make sense to they "fix it" on their end 🤷

Why doesn't next-on-pages ignore/disregard all lambdas?

Lambdas can contain core business logic, so ignoring/disregarding all lambdas would most likely generate applications with incomplete functionalities and it'd very hard to understand what these functionalities are... so that's why we can't simply unconditionally ignore them 😓

dario-piotrowicz commented 4 months ago

@gabrielf I hope the above gives you a bit more context on the problem 🙂

Also note that we point out the fact that the array can't be empty in our Next.js docs page

Please let me know if you have any idea of how we could improve this (dx wise or by making this limitation more discoverable/evident to users) 🙂

gabrielf commented 1 month ago

This is not a crucial issue to fix so I'm closing it. At least this (closed) issue exist for others that may search for it!