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

Throw when parsedUrl is not provided #1015

Merged
merged 4 commits into from
Feb 9, 2017
Merged

Conversation

timneutkens
Copy link
Member

Fixes #990

@timneutkens
Copy link
Member Author

cc @arunoda

@arunoda
Copy link
Contributor

arunoda commented Feb 8, 2017

We should parse this. But we should throw if the user provided something other than a parsedUrl as the third argument.

@timneutkens
Copy link
Member Author

@arunoda This throws in all cases where parsedUrl.query doesn't exist, which is fine since passing it through without true as second parameter will cause a re-parse otherwise, which is unneeded right? Or do you want to keep the url parsing when passing only req, res instead of req, res, parsedUrl?

@arunoda
Copy link
Contributor

arunoda commented Feb 8, 2017

If the parsedUrl is undefined or null, I need to do parse the URL ourselves.

@timneutkens
Copy link
Member Author

@arunoda fixed 🙌

server/index.js Outdated
this.http = http.createServer(this.getRequestHandler())
const handle = this.getRequestHandler()
this.http = http.createServer((req, res) => {
const parsedUrl = parse(req.url, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need this line.

@arunoda arunoda merged commit 53c245b into vercel:master Feb 9, 2017
@arunoda
Copy link
Contributor

arunoda commented Feb 9, 2017

Thanks @timneutkens
This looks great.

@timneutkens timneutkens deleted the 990 branch February 9, 2017 11:07
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants