Skip to content

url: parsed .search should always be a string. #9600

Description

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

Activity

  1. added
    urlIssues and PRs related to the legacy built-in url module.
    on Nov 14, 2016
  2. Trott commented on Nov 14, 2016

    @Trott
    Member

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

  3. jdalton commented on Nov 14, 2016

    @jdalton
    MemberAuthor

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

    Yes.

  4. jalafel commented on Nov 25, 2016

    @jalafel
    Contributor

    Does anyone mind if I take on this issue?

  5. MylesBorins commented on Nov 25, 2016

    @MylesBorins
    Contributor

    @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

  6. jalafel commented on Nov 25, 2016

    @jalafel
    Contributor

    Sounds good to me! Thanks @thealphanerd !

  7. Trott commented on Nov 25, 2016

    @Trott
    Member

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

  8. jalafel commented on Nov 25, 2016

    @jalafel
    Contributor

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

  9. sam-github commented on Nov 25, 2016

    @sam-github
    Contributor

    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?

  10. jdalton commented on Nov 26, 2016

    @jdalton
    MemberAuthor

    The existing behavior seems like a 🐛 to me.

  11. tniessen commented on Jun 2, 2017

    @tniessen
    Member

    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.

  12. apapirovski commented on Oct 19, 2017

    @apapirovski
    Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    urlIssues and PRs related to the legacy built-in url module.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions