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

Resolve undefined url middleware #648

Closed
wants to merge 2 commits into from
Closed

Resolve undefined url middleware #648

wants to merge 2 commits into from

Conversation

jorisdugue
Copy link

Resolve #612

Force to assign Host and Port to Next Server for prevent undefined URL when middleware is enabled

Checklist

index.js Outdated
const app = Next(Object.assign({}, { dev: process.env.NODE_ENV !== 'production' }, nextOptions))
const app = Next(Object.assign({}, {
dev: process.env.NODE_ENV !== 'production',
hostname: hostname !== undefined ? hostname : '0.0.0.0',
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand. Why you need to pass hostname and port to Next?

Copy link
Member

Choose a reason for hiding this comment

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

Really not a good idea to use 0.0.0.0 by default.

Copy link
Author

Choose a reason for hiding this comment

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

Hey, because for obscure reason Next.js need pass port and hostname for prevent the undefined result
According to the docs :

// when using middleware `hostname` and `port` must be provided below
const app = next({ dev, hostname, port })

https://nextjs.org/docs/advanced-features/custom-server

this problem exist too with firebase (without fastfy server ) : vercel/next.js#41210

Copy link
Author

Choose a reason for hiding this comment

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

Really not a good idea to use 0.0.0.0 by default.

i can replace by localhost if you want

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please

middleware.js Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
@andrew-hu368
Copy link

Is there anything blocking the merge of this PR?

if (underPressure) {
const opts = typeof underPressure === 'object' ? underPressure : Object.create(null)

fastify.register(require('@fastify/under-pressure'), opts)
}

const app = Next(Object.assign({}, { dev: process.env.NODE_ENV !== 'production' }, nextOptions))
const app = Next(Object.assign({}, {
Copy link
Member

Choose a reason for hiding this comment

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

After some though, the developer can pass hostname and port directly to the options.
So, when we are helping them to do it. It means that they will never know it is a requirement.

Does middle-ware work when they pass the option through ?

fastify.register(fastifyNext, {
  hostname: 'localhost',
  port: 3000
})

If yes, I am pretty much against this PR.

@climba03003
Copy link
Member

Is there anything blocking the merge of this PR?

Can you try if this works?

fastify.register(fastifyNext, {
  hostname: 'localhost',
  port: 3000
})

@andrew-hu368
Copy link

andrew-hu368 commented Nov 25, 2022

Is there anything blocking the merge of this PR?

Can you try if this works?

fastify.register(fastifyNext, {
  hostname: 'localhost',
  port: 3000
})

It works. We can just update the docs

@jorisdugue jorisdugue closed this Aug 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

There is no host in the middleware of nextjs
3 participants