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

fix(next-server): 'quiet' setting delegate for custom server #64512

Merged
merged 6 commits into from
Apr 17, 2024
Merged

fix(next-server): 'quiet' setting delegate for custom server #64512

merged 6 commits into from
Apr 17, 2024

Conversation

bobaaaaa
Copy link
Contributor

@bobaaaaa bobaaaaa commented Apr 15, 2024

Info

I did not create a GitHub issue, but this PR for fixing the issue. Hope that's okay.

The bug

We run a 'normal' custom server described here: https://nextjs.org/docs/pages/building-your-application/configuring/custom-server

const next = require('next');
const dev = process.env.NODE_ENV !== 'production';
const port = 3000;
const app = next({ dev, port, quiet: !dev })
const handle = app.getRequestHandler()
// ...

The quiet settings is missed in some deeper Next.js internal api calls. For that reason, the setting did not work at all. This works only in production mode.

Testing

I tried to implement a test in /test/production/custom-server but I failed to get the stderr messages. When someone give me some guidance, I will add a new test.

Here are some criteria:

  • need: custom server with quiet: true setting
  • need: a next.js page with thrown error. e.g. export const getServerSideProps = () => { throw new Error('failed') }
  • need: collect all stderr messages

I tested the fix in our project by editing the files in node_modules -- it worked.

Background

We upgrade our project from next 13.3.4 to 14.2.0 and saw this regression. I can not tell in which version this bug was added. We run a Next.js + 'custom server' + Docker setup. Because we have a lot of traffic, we need to suppress the standard Next.js logging and using our own logging.

@ijjk
Copy link
Member

ijjk commented Apr 15, 2024

Allow CI Workflow Run

  • approve CI run for commit: 3eeb78f

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, could we add a test for this similar to our custom server test?

x-ref: https://github.com/vercel/next.js/blob/canary/test/production/custom-server/custom-server.test.ts

@bobaaaaa
Copy link
Contributor Author

bobaaaaa commented Apr 15, 2024

@ijjk I wrote this in the PR description. I tried it in exact this spec, but I failed to get the stderr message collecting to work. When someone give me a guidance, I will do it

@ijjk
Copy link
Member

ijjk commented Apr 15, 2024

Could you push the test you attempted to add and we can work from there?

Comment on lines 18 to 20
it('should not log any error messages when server is started with "quiet" setting', async () => {
await next.render(`/error`)
})
Copy link
Contributor Author

@bobaaaaa bobaaaaa Apr 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ijjk My first impl. was here: https://github.com/vercel/next.js/blob/canary/test/integration/custom-server/test/index.test.js That was super simple. But not for production mode :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They can be set via the env field next to startCommand then the CLI output can be accessed via next.cliOutput.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ijjk Much appreciated for the help. I added two test cases:

  • quite: true -> no logging
  • default quite -> logging

@@ -2,12 +2,13 @@ const { createServer } = require('http')
const { parse } = require('url')
const next = require('next')
const getPort = require('get-port')
const quiet = process.env.USE_QUIET === 'true'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from here:

const { createServer } = require(process.env.USE_HTTPS === 'true'

@ijjk ijjk added the tests label Apr 17, 2024
@ijjk ijjk merged commit 048697b into vercel:canary Apr 17, 2024
91 of 93 checks passed
@bobaaaaa bobaaaaa deleted the fix(next-server)-custom-server-quiet branch April 17, 2024 20:23
@github-actions github-actions bot added the locked label May 2, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants