url: errounous hash appended on url.format

[silverwind]

This is breaking npm installs and should be fixed before 2.0.0.

Encountered in https://github.com/npm/normalize-git-url/blob/master/normalize-git-url.js

1.8.1

> parsed = url.parse("git+ssh://git@github.com:organization/repo.git#hashbrowns")
> parsed.hash = ''
> url.format(parsed)
'git+ssh://git@github.com/:organization/repo.git'

2.0.0

> parsed = url.parse("git+ssh://git@github.com:organization/repo.git#hashbrowns")
> parsed.hash = ''
> url.format(parsed)
> 'git+ssh://git@github.com/:organization/repo.git#'

cc: @petkaantonov @domenic @rvagg

[petkaantonov]

Yea ill fix this in a minute

[domenic]

Is this a bug or is it how browsers behave? Shouldn't you be setting hash to null if you want no hash?

[targos]

On Chrome:

[silverwind]

So this is correct behaviour. location.hash = null also removes the #. I think for a proper fix, we need to get an npm update, I'll open a PR on that repo.

[targos]

Also I do not know how to remove the trailing "#" once it is there.
location.hash = null transforms it to "#null"

[silverwind]

location.hash = null transforms it to "#null"

I'm not seeing this. What are your steps?

[indutny]

I see it too.

[domenic]

This behavior seems to vary per browser. We can check the spec but given that we should maybe pick the most compatible choice for 2.0.0 (i.e. .hash = "" removes the hash) and for 3.0.0 either match the spec or get the spec changed.

[silverwind]

location.hash = null does transform it to #null in my browser (Firefox), but removes the hash completely (including '#') in both io.js 1.8.1 and 2.0.0.

[silverwind]

I think @domenic's suggestion to restore old behaviour is probably the way to go in that case, giving that the null thing isn't conforming either.

[domenic]

So far it seems that for new URL objects, Chrome and Firefox have .hash = null giving #null and .hash = "" removing the hash. But for location, .hash = null gives #null and .hash = "" gives # (Chrome, Firefox, and IE). What a mess.

So yeah I think we should restore the old behavior (definitely for empty string, possibly for null too?) and figure out the longer term story about spec/browser compliance in the future. With more testing of third-party packages this time :)

[silverwind]

@domenic the null case is unchanged with the new parser.

[targos]

Old behavior seems good if I am reading the spec correctly:

The hash attribute’s getter must run these steps:

1. If url is null, or its fragment is either null or the empty string, return the empty string.
2. Return "#" concatenated with fragment.

The hash attribute’s setter must run these steps:

1. If url is null, or its scheme is "javascript", terminate these steps.
2. If the given value is the empty string, set fragment to null, run the pre-update steps, and terminate these steps.
3. Let input be the given value with a single leading "#" removed, if any.
4. Set fragment to the empty string.
5. Basic URL parse input with url as url and fragment state as state override.
6. Run the pre-update steps.

[targos]

From the serializer part (used in href getter):

If the exclude fragment flag is unset and url’s fragment is non-null, append "#" concatenated with url’s fragment to output.

[domenic]

@targos that is not the spec, that is an outdated fork of the spec by another standards organization. Spec is at http://url.spec.whatwg.org/. But yes, the text has not been updated (much) in that section, so your quote is mostly right.

[domenic]

The old behavior is incorrect with regard to .hash = null, since hash is readonly attribute USVString (so any value, including null or undefined, gets converted to a string before processing). But we can fix that later, maybe.

[targos]

@domenic ok thanks

btw the href property is not always reset:

> var uri = url.parse('http://www.example.com')
undefined
> uri.href
'http://www.example.com/'
> uri.hash = 'test'
'test'
> uri.href
'http://www.example.com/#test'
> uri.hash = null
null
> uri.href
'http://www.example.com/#test'
> uri.format()
'http://www.example.com/'

Is this intended ?

[silverwind]

@targos probably not. .href was never updated in the old implementation, by the way:

> process.version
'v1.8.1'
> var uri = url.parse('http://www.example.com')
undefined
> uri.href
'http://www.example.com/'
> uri.hash = 'test'
'test'
> uri.href
'http://www.example.com/'
> uri.hash = null
null
> uri.href
'http://www.example.com/'
> uri.format()
'http://www.example.com/'
> uri.hash = 'test'
'test'
> uri.format()
'http://www.example.com/#test'

[petkaantonov]

yes href not always updating is a bug, i'll fix that as well

[petkaantonov]

Fixed in 6687721