url: parsed .search should always be a string.

[jdalton]

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.

[Trott]

I guess the same would go for hash, port and probably others...

[jdalton]

I guess the same would go for hash, port and probably others...

Yes.

[jalafel]

Does anyone mind if I take on this issue?

[MylesBorins]

@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]

Sounds good to me! Thanks @thealphanerd !

[Trott]

@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_issues and into test/parallel.

Doing it in two parts like that is not required, but it's an option.

[jalafel]

@Trott Thanks! I'm interested in keeping with due diligence to the process. So I will break it up into two!

[sam-github]

I don't think we should change url, existing behaviour is justifiable, and @jasnell is working on a url.URL that will meet browser specifications. Why break people's existing code, when we still wouldn't get a browser compliant url lib?

[jdalton]

The existing behavior seems like a 🐛 to me.

[tniessen]

I agree with @sam-github:

existing behaviour is justifiable, and @jasnell is working on a url.URL that will meet browser specifications

Interpreting null as "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:

While the Legacy API has not been deprecated, it is maintained solely for backwards compatibility with existing applications. New application code should use the WHATWG API.

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]

Since there are unlikely to be changes like this on the old parse API 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. :)