Skip to content

url: errounous hash appended on url.format #1588

Closed
Closed
url: errounous hash appended on url.format#1588
Assignees
petkaantonov
Labels
confirmed-bugIssues and PRs for confirmed bugs.urlIssues and PRs related to the legacy built-in url module.
Milestone
2.0.0

Description

@silverwind
silverwind
opened on May 2, 2015

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

Activity

  1. silverwind
    added
    confirmed-bugIssues and PRs for confirmed bugs.
    urlIssues and PRs related to the legacy built-in url module.
    on May 2, 2015
  2. silverwind
    added this to the 2.0.0 milestone on May 2, 2015

  3. petkaantonov commented on May 2, 2015

    @petkaantonov
    petkaantonov
    on May 2, 2015
    Contributor

    Yea ill fix this in a minute

  4. domenic commented on May 2, 2015

    @domenic
    domenic
    on May 2, 2015
    Contributor

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

  5. targos commented on May 2, 2015

    @targos
    targos
    on May 2, 2015
    Member

    On Chrome:

    capture

  6. silverwind commented on May 2, 2015

    @silverwind
    silverwind
    on May 2, 2015
    ContributorAuthor

    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.

  7. targos commented on May 2, 2015

    @targos
    targos
    on May 2, 2015
    Member

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

  8. silverwind commented on May 2, 2015

    @silverwind
    silverwind
    on May 2, 2015
    ContributorAuthor

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

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

  9. silverwind
    added a commit that references this issue on May 2, 2015

    Properly remove the hash part of the url

    3ea135b

  10. indutny commented on May 2, 2015

    @indutny
    indutny
    on May 2, 2015
    Member

    I see it too.

  11. domenic commented on May 2, 2015

    @domenic
    domenic
    on May 2, 2015
    Contributor

    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.

  12. silverwind commented on May 2, 2015

    @silverwind
    silverwind
    on May 2, 2015
    ContributorAuthor

    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.

  13. silverwind commented on May 2, 2015

    @silverwind
    silverwind
    on May 2, 2015
    ContributorAuthor

    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.

  14. domenic commented on May 2, 2015

    @domenic
    domenic
    on May 2, 2015
    Contributor

    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 :)

  15. 1 remaining item

  16. targos commented on May 2, 2015

    @targos
    targos
    on May 2, 2015
    Member

    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.

  17. targos commented on May 2, 2015

    @targos
    targos
    on May 2, 2015
    Member

    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.

  18. domenic commented on May 2, 2015

    @domenic
    domenic
    on May 2, 2015
    Contributor

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

  19. domenic commented on May 2, 2015

    @domenic
    domenic
    on May 2, 2015
    Contributor

    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.

  20. targos commented on May 2, 2015

    @targos
    targos
    on May 2, 2015
    Member

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

  21. petkaantonov
    self-assigned this
    on May 2, 2015

  22. silverwind commented on May 2, 2015

    @silverwind
    silverwind
    on May 2, 2015
    ContributorAuthor

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

  23. petkaantonov commented on May 2, 2015

    @petkaantonov
    petkaantonov
    on May 2, 2015
    Contributor

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

  24. laiso
    added a commit that references this issue on May 2, 2015

    Avoid git-clone with an invalid URL

    1a6b855
  25. petkaantonov
    added a commit that references this issue on May 2, 2015

    url: fix treatment of some values as non-empty

    e274b92
  26. petkaantonov
    added 2 commits that reference this issue on May 3, 2015

    url: delete href cache on all setter code paths

    dbdd81a

    url: fix treatment of some values as non-empty

    6687721

  27. petkaantonov commented on May 3, 2015

    @petkaantonov
    petkaantonov
    on May 3, 2015
    Contributor

    Fixed in 6687721

  28. petkaantonov
    closed this as completedon May 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

confirmed-bugIssues and PRs for confirmed bugs.urlIssues and PRs related to the legacy built-in url module.

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions