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 124 forks source link

[🐛 Bug]: Incompatible with Next.js instrumentation #678

Open Maronato opened 8 months ago

Maronato commented 8 months ago

next-on-pages environment related information

 System:
        Platform: darwin
        Arch: arm64
        Version: Darwin Kernel Version 23.2.0: Wed Nov 15 21:55:06 PST 2023; root:xnu-10002.61.3~2/RELEASE_ARM64_T6020
        CPU: (12) arm64 Apple M2 Max
        Memory: 64 GB
        Shell: /bin/zsh
Package Manager Used: npm (9.8.1)

Relevant Packages:
        @cloudflare/next-on-pages: 1.9.0
        vercel: 33.5.2
        next: 14.1.0

Description

next-on-pages breaks when using Next's default instrumentation @vercel/otel, resulting in the following error:

... rest of next build
⚡️ Completed `npx vercel build`.

⚡️ ERROR: A duplicated identifier has been detected in the same function file, aborting.

⚡️ Please report this at https://github.com/cloudflare/next-on-pages/issues.

Reproduction

Repo: https://github.com/Maronato/next-on-pages-instrumentation-repro

Steps to reproduce:

  1. Create a new project with npm create cloudflare@latest next-on-pages -- --framework=next
  2. Install @vercel/otel (npm install @vercel/otel)
  3. Add experimental: { instrumentationHook: true } to your next config
  4. Create a new file in the src or root of the project called instrumentation.ts
  5. Paste the following contents:
    
    // src/instrumentation.ts

import { registerOTel } from "@vercel/otel"

export function register() { registerOTel() }


6. Build with `npx next-on-pages`

### Pages Deployment Method

None

### Pages Deployment ID

_No response_

### Additional Information

The error seems to happen in [this function](https://github.com/cloudflare/next-on-pages/blob/be1ab75a0a5214e047c5a0cbb09d657b99e46bfc/packages/next-on-pages/src/buildApplication/processVercelFunctions/ast.ts#L68-L104), but I'm not sure why.

A few notes:
- Calling `registerOTel` is not required. Simply printing it (`console.log(registerOTel)`) is enough to cause the error
- Vercel made the repo private, so I can't inspect the code https://github.com/vercel/otel
- The code that they ship is minified
- They [say](https://nextjs.org/docs/pages/building-your-application/optimizing/open-telemetry#manual-opentelemetry-configuration) that you can use OTEL's libraries directly but it only works with Node. To use with edge you have to use their library

### Would you like to help?

- [ ] Would you like to help fixing this bug?
ostenbom commented 7 months ago

If I run next-on-pages with --disableChunksDedup, it works ok. The duplicate identifier seems to be webpack! Is it safe to run un-deduped builds? 😄

james-elicx commented 7 months ago

If I run next-on-pages with --disableChunksDedup, it works ok. The duplicate identifier seems to be webpack! Is it safe to run un-deduped builds? 😄

It's safe to, you'll just have a much much larger bundle size

jan-acemate commented 3 months ago

Any update on this? @Maronato were you able to find a workaround other than disabling the ChunksDedup?

jan-acemate commented 3 months ago

Just leaving some insights, in case anyone is interested:

As @ostenbom indicated, the above error seems to originate from the Webpack module ID assignment. Overwriting the default assignment algorithm fixes the error; however, it results in a much larger bundle size (a factor of 8 in my case), which usually makes the bundle exceed Cloudflare's maximum limit of 25 MB.

Example that fixes the error, but bloats the bundle size:

//next.config.js
  webpack: (config) => {
    config.optimization = {
      ...config.optimization,
      moduleIds: false, // Disable built-in module ID algorithms
    };
    config.plugins.push(new webpack.ids.NaturalModuleIdsPlugin());
  }

I tried all algorithms listed in the webpack optimization documentation, but all of them either cause the build error or led to a significant increase in bundle size. Trying other options to reduce the bundle size did not make a significant difference.

This is unfortunate, as this is by far the most convenient way of adding observability to a Next.js application. From Cloudflare's perspective, fixing this would especially make sense given their acquisition of Baselime, which is currently almost useless for Cloudflare Pages (except for Edge logging). I hope this issue will be addressed at some point.