fix: Avoid merging Location header on response when its an array

[sommeeeer]

This is for opennextjs/opennextjs-cloudflare#875 (comment)

Next will return a header like Location: ["/target", "/target"]; on the response for a redirect("/target"); in a static page.tsx when you dont have an incrementalCache. We should not merge that header's value with join(",");

Affected line in Next: https://github.com/vercel/next.js/blob/ea08bf27/packages/next/src/server/base-server.ts#L1521

This is me console.log(res._res.headers) before and after that line with a reproduction:

[changeset-bot]

🦋 Changeset detected

Latest commit: de49e88

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@opennextjs/aws	Patch
app-pages-router	Patch
app-router	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[pkg-pr-new]

Open in StackBlitz

pnpm add https://pkg.pr.new/@opennextjs/aws@977

commit: de49e88

[vicb]

Thanks for putting this PR together @sommeeeer !

A few questions as you have looked into this:

Do I understand correctly than Next doesn't really care about what's returned when there is no incremental cache... as they always have an incremental cache?

Should we rather error when we have static/cached content and no incremental cache?

If the header value is an array [location1, location2], what are location1 and location2?
Are there always exactly 2 locations?
Are location1 and location2 always the same (i.e. why should we pick [0])

[sommeeeer]

Do I understand correctly than Next doesn't really care about what's returned when there is no incremental cache... as they always have an incremental cache?

Yeah, or they should SSR that page then, but for some reason in this case the Location's value is an array. Well, when I think more about this. Why would you ever want to setup an OpenNext/Next project without ISR. You could just use the static export then. Perhaps I should close this PR? WDYT?

Are location1 and location2 always the same (i.e. why should we pick [0])

Yeah, they are always the same. I guess somewhere in Next's code they put that header value twice when get returns null from the cacheHandler.

[vicb]

Do I understand correctly than Next doesn't really care about what's returned when there is no incremental cache... as they always have an incremental cache?

Yeah, or they should SSR that page then, but for some reason in this case the Location's value is an array. Well, when I think more about this. Why would you ever want to setup an OpenNext/Next project without ISR. You could just use the static export then. Perhaps I should close this PR? WDYT?

Are location1 and location2 always the same (i.e. why should we pick [0])

Yeah, they are always the same. I guess somewhere in Next's code they put that header value twice when get returns null from the cacheHandler.

Not sure what is the best way to handle this.

Maybe the Open Next build should fail when Next has cache artifacts and Open Next has no cache configured?
There should be no reason to force configuring cache for a fully dynamic app.

I think it's worth leaving this PR opened until we discuss more about it.

[sommeeeer]

Maybe the Open Next build should fail when Next has cache artifacts and Open Next has no cache configured?

Thats actually a great idea, however I think we should discuss this further as you mentioned. This decision can have consequences and needs to be thought about thoroughly.

I'll leave it open for now.

[conico974]

Maybe the Open Next build should fail when Next has cache artifacts and Open Next has no cache configured?

It should be a big warning not an error. If your app is fully SSR, it could make sense to have no Incremental cache.
And splitted function don't necessarily need the cache either.

This error could also happen if you have a cache configured, but it errors out. In this case we return null from the cache handler (same thing as with the dummy incremental cache) and we have this bug

As to what exactly to do here, before picking the value, we should check if they are the same, if it's the case we're fine.
If not, maybe pick the last one (logically the last one should be the latest added there), but we should log something here.

[sommeeeer]

Thanks for the reviews!

As to what exactly to do here, before picking the value, we should check if they are the same, if it's the case we're fine. If not, maybe pick the last one (logically the last one should be the latest added there), but we should log something here.

Alright, I addressed this in a review commit.

[vicb]

are we sure they are always exactly 2 values in the array?

[sommeeeer]

are we sure they are always exactly 2 values in the array?

Yeah, and it could only happen if it returns null from a get in the cacheHandler.

[vicb]

Maybe you can add the info in the JS doc ?