Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extending request while using a custom server/standalone mode #54440

Closed
1 task done
balazsorban44 opened this issue Aug 23, 2023 · 19 comments · Fixed by #54813
Closed
1 task done

Extending request while using a custom server/standalone mode #54440

balazsorban44 opened this issue Aug 23, 2023 · 19 comments · Fixed by #54813
Labels
linear: next Confirmed issue that is tracked by the Next.js team. locked Navigation Related to Next.js linking (e.g., <Link>) and navigation. Output (export/standalone) Related to the the output option in `next.config.js`. Pages Router Related to Pages Router.

Comments

@balazsorban44
Copy link
Member

balazsorban44 commented Aug 23, 2023

Verify canary release

  • I verified that the issue exists in the latest Next.js canary release

Provide environment information

Versions of Next.js 13.4.13 and above are affected.

Which area(s) of Next.js are affected? (leave empty if unsure)

Data fetching (gS(S)P, getInitialProps), Routing (next/router, next/navigation, next/link), Standalone mode (output: "standalone")

Link to the code that reproduces this issue or a replay of the bug

Reproductions are available in the linked issues below

To Reproduce

Reproductions are available in the linked issues below

Describe the Bug

When using a custom server/standalone mode, there might be an incentive to extend the request that is then passed on to other parts of the application to access the extra property.

Since 13.4.13, this is not straightforward/possible.

Link to related issues to consolidate them in a single location for tracking:

Expected Behavior

This is a bit tricky to solve because of the render workers, but we would like to track this until we have an answer/solution.

NOTE

If you are affected, please leave a comment below with your use case and a reproduction of the issue (make it as minimal as possible, so it's easy to investigate to anyone without prior knowledge of your codebase). Please do not comment without meaningful additional information (if you want to comment "same", etc., react 👍 to the original description instead)! Thank you.

Unhelpful/unrelated comments will be marked as off-topic/spam to keep the readability of the thread.

NEXT-1553

@balazsorban44 balazsorban44 added the bug Issue was opened via the bug report template. label Aug 23, 2023
@github-actions github-actions bot added Pages Router Related to Pages Router. Navigation Related to Next.js linking (e.g., <Link>) and navigation. Output (export/standalone) Related to the the output option in `next.config.js`. labels Aug 23, 2023
@balazsorban44 balazsorban44 added linear: next Confirmed issue that is tracked by the Next.js team. and removed bug Issue was opened via the bug report template. labels Aug 23, 2023
@itrelease

This comment was marked as off-topic.

@t-net

This comment was marked as off-topic.

@coltonehrman

This comment was marked as spam.

@coltonehrman

This comment was marked as spam.

@skogie

This comment was marked as resolved.

@coltonehrman
Copy link
Contributor

I found some interesting stuff regarding the issues in 13.4.13

Initial PR
#52492

Reverted before 13.4.12 release
#53016

Screenshot 2023-08-25 at 9 17 21 AM

Reverted Revert
#53029

No trace of when or where the reverted revert got released
It's as if this change got snuck in to 13.4.13

My hunch is this is what "broke the world", separating the router & render processes basically broke all things being passed between custom server (runs before routing) and render (runs in Next.js)

@coltonehrman
Copy link
Contributor

coltonehrman commented Aug 25, 2023

Tracing logic:

invokeRequest get's called:
https://github.com/vercel/next.js/blob/canary/packages/next/src/server/lib/router-server.ts#L378-L386

fetch get's called:
https://github.com/vercel/next.js/blob/canary/packages/next/src/server/lib/server-ipc/invoke-request.ts#L22-L41

Not sure how this worked before, but based on this logic, the extended res or req object will NEVER get sent to the "render" as it is making a new http request internally. This design of "separation" inherently will not support passing data between the custom server and Next.js router/renderer.

@nicholasgriffintn

This comment was marked as off-topic.

@coltonehrman
Copy link
Contributor

coltonehrman commented Aug 25, 2023

No trace of when or where the reverted revert got released
It's as if this change got snuck in to 13.4.13

The reverted revert was the first commit in v13.4.13-canary.0.

Yeah, it appears so. They should update the release notes here https://github.com/vercel/next.js/releases/tag/v13.4.13-canary.0 and here https://github.com/vercel/next.js/releases/tag/v13.4.13

This is arguably the BIGGEST change of the whole release and likely should have been a major/minor update. Not patch.

@grawk
Copy link

grawk commented Aug 28, 2023

Hey maintainers. This is a radical departure in terms of functionality for teams using Next.js with an express custom server, and relying upon decorated req/res objects. Can you all please provide a statement soon about whether this loss of functionality was purposeful and will not be maintained going forward? It is critical to helping with my team's decision of whether to continue using Next.js for our use case.

@nicholasgriffintn
Copy link

It is critical to helping with my team's decision of whether to continue using Next.js for our use case.

^ agreed, and I'd add, it seems that related functionality for custom server isn't being tested or validated, or considered for breaking changes, it would be good if it was.

If it's stated that next doesn't support custom server then that is also fine though.

@timneutkens
Copy link
Member

timneutkens commented Aug 29, 2023

This is a bug. We're figuring out how to bring this back as it's non-trivial to support the request object being modified, it's accidental that it broke, there was no test for that, the tests for using the custom server API test the documented behavior which is passing Node.js request / response into the handler.

@coltonehrman
Copy link
Contributor

coltonehrman commented Aug 29, 2023

This is a bug. We're figuring out how to bring this back as it's non-trivial to support the request object being modified, it's accidental that it broke, there was no test for that, the tests for using the custom server API test the documented behavior which is passing Node.js request / response into the handler.

Will the bug fix only fix the req/res extension use case?

Based on the new code & design, this is just one case that is broken, other cases would include any bootstrapping of global objects at the custom server level & singleton modules that get required and depended on in the custom server and within Next.js contexts.

@coltonehrman
Copy link
Contributor

This is a bug. We're figuring out how to bring this back as it's non-trivial to support the request object being modified, it's accidental that it broke, there was no test for that, the tests for using the custom server API test the documented behavior which is passing Node.js request / response into the handler.

Will the bug fix only fix the req/res extension use case?

Based on the new code & design, this is just one case that is broken, other cases would include any bootstrapping of global objects at the custom server level & singleton modules that get required and depended on in the custom server and within Next.js contexts.

I have opened a new issue related to the other bugs #54782

@cosieLq
Copy link

cosieLq commented Sep 6, 2023

Another change that's happened in #53029
is that the internal routing for static assets (/_next/static).

As mentioned in #54159 , custom response headers in next config are not applied to static assets when i18n is configured.
Since v13.4.13, NextJs also shows this inconsistency in applying custom headers for static assets between with and without i18n.

I hope this issue will be fixed soon as well. It has become a blocker for us.

@shaunwarman
Copy link

This is a bug. We're figuring out how to bring this back as it's non-trivial to support the request object being modified, it's accidental that it broke, there was no test for that, the tests for using the custom server API test the documented behavior which is passing Node.js request / response into the handler.

@timneutkens would you mind sharing what specific change was made here to get eyes / brainstorming?

timneutkens added a commit that referenced this issue Sep 11, 2023
Currently we create separate workers to isolate `pages` and `app`
routers due to differing react versions being used between the two. This
adds overhead and complexity in the rendering process which we can avoid
by leveraging an `esm-loader` similar to our `require-hook` to properly
alias `pages` router to the bundled react version to match `app` router
when both are leveraged together.

This aims to seamlessly inject the `esm-loader` by restarting the
process with the loader arg added whenever `next` is imported so that
this also works with custom-servers and fixes the issue with custom
req/res fields not working after upgrading.


x-ref: #53883
x-ref: #54288
x-ref: #54129
x-ref: #54435
closes: #54440
closes: #52702
x-ref: [slack
thread](https://vercel.slack.com/archives/C03KAR5DCKC/p1693348443932499?thread_ts=1693275196.347509&cid=C03KAR5DCKC)

---------

Co-authored-by: Tim Neutkens <tim@timneutkens.nl>
Co-authored-by: Zack Tanner <zacktanner@gmail.com>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
@nicholasgriffintn
Copy link

nicholasgriffintn commented Sep 11, 2023

Just tried it out real quick, getting an error NextRouter was not mounted on our app using pages with 13.4.20-canary.24, not getting it with 13.4.20-canary.23.

@nicholasgriffintn
Copy link

^ Update: Changing useRouter imports in the packages that were erroring to use next/compat/router seems to have sorted the issue, it seems the change made means that next/router imports no longer work from packages for some reason.

@github-actions
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for 2 weeks. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
linear: next Confirmed issue that is tracked by the Next.js team. locked Navigation Related to Next.js linking (e.g., <Link>) and navigation. Output (export/standalone) Related to the the output option in `next.config.js`. Pages Router Related to Pages Router.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants