Repository navigation
url: parsed .search should always be a string. #9600
Description
Node v7.1.0.
I parsed a url with the require('url') module using .parse and ran into a gotcha.
I had path.join('vendor', url.pathname, url.search) and it errored because .search was null.
I had assumed it followed the browser behavior of returning an empty string when no search query is found.
Activity
Trott commentedon Nov 14, 2016
I guess the same would go for
hash,portand probably others...jdalton commentedon Nov 14, 2016
Yes.
jalafel commentedon Nov 25, 2016
Does anyone mind if I take on this issue?
MylesBorins commentedon Nov 25, 2016
@jessicaquynh this may be a good issue to dig into but @jasnell should be able to confirm if this is something that will be an easy fix, or something we rely on the WhatWG implementation for
jalafel commentedon Nov 25, 2016
Sounds good to me! Thanks @thealphanerd !
Trott commentedon Nov 25, 2016
@jessicaquynh If you want, this can be broken up into two contributions: First, write tests for these and put them in
test/known_issues(which is where we put tests that we expect to fail but that we one day hope to move to the regular test suite when we fix issues in the code such that they pass).Then you can contribute a fix that would also move those tests out of
test/known_issuesand intotest/parallel.Doing it in two parts like that is not required, but it's an option.
jalafel commentedon Nov 25, 2016
@Trott Thanks! I'm interested in keeping with due diligence to the process. So I will break it up into two!
test: known issue url.parse() does not return an empty string
sam-github commentedon Nov 25, 2016
I don't think we should change url, existing behaviour is justifiable, and @jasnell is working on a
url.URLthat will meet browser specifications. Why break people's existing code, when we still wouldn't get a browser compliant url lib?jdalton commentedon Nov 26, 2016
The existing behavior seems like a 🐛 to me.
tniessen commentedon Jun 2, 2017
I agree with @sam-github:
Interpreting
nullas "nonexistent" is a reasonable justification of the current behavior. Changing this behavior would likely break a lot of code, and since v7.0.0 the docs explicitely say:The new API provides the desired behavior and changes to the old API will most probably do more harm than good at this point.
apapirovski commentedon Oct 19, 2017
Since there are unlikely to be changes like this on the old
parseAPI and there hasn't been any movement on this in a long time (in fact we've gone in the opposite direction with some bug fixes), I'm going to go ahead and close this. That said, please feel free to reopen if you believe this is still an issue that needs to be addressed — I'm just trying to keep things tidy and not acting on any strong opinions. :)