sst / open-next

Open source Next.js serverless adapter
https://open-next.js.org
MIT License
3.7k stars 111 forks source link

Image optimization fails due to version mismatch #406

Closed Seanw265 closed 1 month ago

Seanw265 commented 2 months ago

I'm using OpenNext (v2.3.7) through the SST NextJsSite construct.

I recently had an issue where images loaded through the <Image> tag failed at the optimization step. After digging into the issue, I learned that Next changed the optimizer call signature in v14.1.1, hence #374 and #377. I was running v14.2.2.

Digging in further, I learned that the OpenNext image optimization plugin was not applied during my build step. This is because the plugin is gated behind a version check for v14.1.1. BUT when a Next version isn't specified in the initial options, OpenNext attempts to extract it from package.json.

However, the version specified in package.json is not necessarily the version that's installed. In my case, I had "next": "^14.0.4" in my package.json, but the actual installed version was v14.2.2. This resulted in a logic mismatch between versions, and the image optimizer function simply returned Cannot read properties of undefined (reading '0') every time it was invoked.

It seems like an appropriate solution would be to instead obtain the installed version number from package-lock.json or the module itself, but I defer to the original authors.

ETA: I was able to fix my original issue by bumping the next version in package.json to ^14.2.2.

conico974 commented 2 months ago

I think the current solution is the best we can do without overcomplicating things, and to me having two different versions in package.json and package-lock.json is more an issue ( Maybe i'm missing something here )

We cannot just use package-lock.json, if we'd do that we need to support pnpm-lock.yaml and yarn.lock and the bun lockfile ( which is a binary files ). To add even more complexity to that in a monorepo settings, you could have multiple next app with different versions, all in a single lockfile.

The other solutions could be to use require("next/package.json").version, but this won't work for cases where you're running OpenNext from a different directory than next ( which is possible )

khuezy commented 2 months ago

@Seanw265 thanks for the investigation, you have a good point... but to add to what @conico974 said: nextjs has been known to have breaking changes even in their .patch releases. They don't adhere semversioning, so I'd recommend fixing your next version instead of using ^.

To be safe, you should use ~ instead of ^, but in general, especially with node, you should fix your packages b/c broken minor/patches in node happens more than you think.

Seanw265 commented 2 months ago

@conico974 @khuezy thanks for your response.

To be safe, you should use ~ instead of ^, but in general, especially with node, you should fix your packages b/c broken minor/patches in node happens more than you think.

Please correct me if I'm wrong, but I believe this issue would still have occurred even if I had used ~ in package.json if it contained "next":"~14.1.0". That aside, using ^ and ~ is not out of the ordinary. Vercel itself uses it in their monorepo example which is provided as a template.

But I can certainly understand where you're coming from. Without delving in too deep, our deployment process is set up in such a way that we would catch a breaking change before it's released, but I think we're the exception and not the rule.


As more projects adopt OpenNext and as Vercel releases new breaking changes to Nextjs that need to be addressed with plugins, this could become a larger issue.

Perhaps a low-effort solution would be to add something to the documentation that specifies that the next package should be pinned due to a reliance on the version number specified in package.json; failure to do so could result in unexpected behavior, as demonstrated in this ticket.

Going a step further, OpenNext could log a warning during the build step if the app's next version is not pinned. This should be simpler than identifying the installed version of next, while being more robust than a simple documentation change.

khuezy commented 2 months ago

Yea, that wouldn't have prevented the issue, but it'll minimize packages that have breaking changes in the minor version. We should look into this to improve actual version detection.

kelvinCJJ commented 2 months ago

@Seanw265 I tried to update it to nextjs 14.1. but still getting "Cannot read properties of undefined (reading '0')"

conico974 commented 2 months ago

@kelvinCJJ Are you using windows ?

kelvinCJJ commented 2 months ago

Yes I am using windows and downloaded WSL, but it still doesnt seem to work

conico974 commented 2 months ago

@kelvinCJJ What version of next is inside package.json ? ( With ^ or ~ if they're here ) And you run open-next build from within WSL ? Using linux filesystem or windows ?

kelvinCJJ commented 2 months ago

Not sure of the actual issue, but downgrading to next@14.1.0 solved it for me

conico974 commented 2 months ago

@kelvinCJJ That's not really a fix, if you see this error it very likely means that the plugin system used internally don't work. If that's the case, it's very likely that a lot of things might break in production ( ISR, On demand revalidation, Middleware etc... ) https://github.com/sst/open-next/issues/385#issuecomment-1999016215

kelvinCJJ commented 2 months ago

I am using nextjs ^14.1.0 on windows. I have installed WSL and currently only use sst dev and sst deploy in my cli. How do I run open-next build from within WSL?

conico974 commented 2 months ago

@kelvinCJJ I realize i forgot to ask you this, but which version of OpenNext are you using ?

If you didn't change the buildCommand in the sst constructs open-next build already runs on WSL.

But you missed one very important point here, you should pin your version of next in package.json ( i.e. no ^ and no ~ and run install again ) With what you have if you remove your lockfile and install again it will install next@14.2.3 Also as stated in the comment i linked, you very likely want to move your repo to the WSL filesystem and work only there ( or use some CI to deploy like github actions )

And if you want to check if the plugin system is running fine, run OPEN_NEXT_DEBUG=true npx open-next@2.3.9 build inside WSL and post the result here

kelvinCJJ commented 2 months ago

I am using 2.3.9. I did not change the buildCommand in the nextjs construct. I will take note of the next version

I already have wsl installed and deployed as per normal without any changes in the build command. This should mean I am running it in WSL yeah?

Update: DEBUG Applying plugins:: [opennext-13.5-serverHandler,opennext-13.5-util,opennext-13.5-default] for Next version: 14.1.0

conico974 commented 2 months ago

@kelvinCJJ If that's the only output you get, then it's broken. This is what you're supposed to have

DEBUG Applying plugins:: [opennext-13.5-serverHandler,opennext-13.5-util,opennext-13.5-default] for Next version: 14.1.0
DEBUG Open-next plugin opennext-13.5-serverHandler -- Applying override for imports from ./13.5/serverHandler.js
DEBUG Open-next plugin opennext-13.5-serverHandler -- Applying override for handler from ./13.5/serverHandler.js
DEBUG Open-next plugin opennext-13.5-default -- Applying override for imports from ./default.replacement.js
DEBUG Open-next plugin opennext-13.5-default -- Applying override for processInternalEvent from ./default.replacement.js
DEBUG Open-next plugin opennext-13.5-default -- Applying override for postProcessResponse from ./default.replacement.js
DEBUG Open-next plugin opennext-13.5-util -- Applying override for requestHandler from ./14.1/util.js
DEBUG Open-next plugin opennext-13.5-util -- Applying override for requireHooks from ./14.1/util.js

My advice, just use github actions

Seanw265 commented 2 months ago

@kelvinCJJ and anyone else who experiences this issue:

I'm simplifying things a bit, but this should cover most cases. You have two options.

Option 1

You may "pin" the next package to a specific version. This means that your package.json entry for next should NOT contain ~ or ^.

For example: "next":"14.1.0""next":"^14.1.0" ❌ no good "next":"~14.1.0" ❌ no good

Option 2

If you must continue to use ~ or ^ in your package.json file, you must set the numeric portion of your version to something >= 14.1.1.

For example: "next":"~14.1.1""next":"^14.1.1""next":"^14.2.0""next":"~14.1.0" ❌ no good "next":"^14.0.4" ❌ no good


Afterwards

After adjusting your package.json file, you should consider performing a clean install of all of your dependencies.