Skip to content

path: iojs@1.6.0 breaks compatibility with previous versions #1215

Closed
Closed
path: iojs@1.6.0 breaks compatibility with previous versions#1215
Labels
confirmed-bugIssues and PRs for confirmed bugs.pathIssues and PRs related to the path subsystem.

Description

@feross
feross
opened on Mar 20, 2015

In iojs 1.5.1:

> path.dirname(undefined)
'.'

> path.dirname([ 'sup/dude' ])
'sup'

In iojs 1.6.0:

> path.dirname(undefined)
TypeError: Path must be a string. Received undefined
    at assertPath (path.js:8:11)
    at Object.posix.dirname (path.js:539:3)
    at repl:1:6
    at REPLServer.defaultEval (repl.js:124:27)
    at bound (domain.js:254:14)
    at REPLServer.runBound [as eval] (domain.js:267:12)
    at REPLServer.<anonymous> (repl.js:277:12)
    at emitOne (events.js:77:13)
    at REPLServer.emit (events.js:166:7)
    at REPLServer.Interface._onLine (readline.js:195:10)

> path.dirname([ 'sup/dude' ])
TypeError: Path must be a string. Received [ 'sup/dude' ]
    at assertPath (path.js:8:11)
    at Object.posix.dirname (path.js:539:3)
    at repl:1:6
    at REPLServer.defaultEval (repl.js:124:27)
    at bound (domain.js:254:14)
    at REPLServer.runBound [as eval] (domain.js:267:12)
    at REPLServer.<anonymous> (repl.js:277:12)
    at emitOne (events.js:77:13)
    at REPLServer.emit (events.js:166:7)
    at REPLServer.Interface._onLine (readline.js:195:10)

This appears to be a bug introduced by this PR: #1153.

This suddenly started causing tests in webtorrent and browserify to start failing. Here's an example.

Activity

  1. rvagg commented on Mar 20, 2015

    @rvagg
    rvagg
    on Mar 20, 2015
    Member

    /cc @cjihrig

  2. cjihrig commented on Mar 20, 2015

    @cjihrig
    cjihrig
    on Mar 20, 2015
    Contributor

    Looking.

  3. rvagg commented on Mar 20, 2015

    @rvagg
    rvagg
    on Mar 20, 2015
    Member

    workflow

    This is all I can think of here, expose any API to a large enough audience and you're sure to have some of them using unexpected edge-cases as an integral part of their workflow.

    I'm not convinced we should "fix" this but perhaps in-the-wild use is reason enough?

    / @iojs/tc please weigh in

  4. cjihrig commented on Mar 20, 2015

    @cjihrig
    cjihrig
    on Mar 20, 2015
    Contributor

    The case for undefined returning '.' makes some amount of sense. The array usage does not. Neither of these uses are documented or tested. How do we want to proceed?

  5. rvagg commented on Mar 20, 2015

    @rvagg
    rvagg
    on Mar 20, 2015
    Member

    I don't have a strong opinion on this and am tempted to call out those relying on edge-cases to fix bugs in their own code, but perhaps that's not very community-friendly of us and we should just be paving cowpaths!

  6. rvagg commented on Mar 20, 2015

    @rvagg
    rvagg
    on Mar 20, 2015
    Member

    Alternative here is to print a deprecation warning on anything other than strings, that might be the gentle way to proceed. I honestly can't make sense of path.dirname([ 'sup/dude' ]).

  7. johnsoftek commented on Mar 20, 2015

    @johnsoftek
    johnsoftek
    on Mar 20, 2015

    +1 for deprecation warning

  8. feross commented on Mar 20, 2015

    @feross
    feross
    on Mar 20, 2015
    ContributorAuthor

    The array use case is weird – agreed. I have no idea how/why that was working before.

    path.dirname(undefined) returning '.' makes more sense, though.

  9. feross commented on Mar 20, 2015

    @feross
    feross
    on Mar 20, 2015
    ContributorAuthor

    Ah, I figured out why the array parameter was working:

    > path.dirname([ 'sup/dude', 'sup2/dude2' ])
'sup/dude,sup2'

    The array was being toStringed. This was definitely a bug in webtorrent.

    I still think we should consider making path.dirname(undefined) work like before.

  10. cjihrig commented on Mar 20, 2015

    @cjihrig
    cjihrig
    on Mar 20, 2015
    Contributor

    Some functions flat out break if a string isn't passed. I'm trying to identify those, and will remove the check from the others.

  11. brendanashworth
    added
    confirmed-bugIssues and PRs for confirmed bugs.
    pathIssues and PRs related to the path subsystem.
    on Mar 20, 2015

  12. rvagg commented on Mar 20, 2015

    @rvagg
    rvagg
    on Mar 20, 2015
    Member

    I'll put this in the 1.6.1 bucket if we can get it done today, is that reasonable @cjihrig?

  13. cjihrig commented on Mar 20, 2015

    @cjihrig
    cjihrig
    on Mar 20, 2015
    Contributor

    Coming in a few minutes.

  14. silverwind commented on Mar 20, 2015

    @silverwind
    silverwind
    on Mar 20, 2015
    Contributor

    I'm more in the camp of not supporting weird usage like path.dirname(undefined). This is definitely a user error which we mask by returning something.

    This strongly reminds me of JS type coercion. Sometimes it's better to crash and burn, than to silently try to do the right thing™.

  15. 4 remaining items

  16. domenic commented on Mar 20, 2015

    @domenic
    domenic
    on Mar 20, 2015
    Contributor

    FWIW all string-accepting web APIs and built-in ES APIs to ToString on their arguments, instead of type-testing. I am surprised io.js is trying to break with that; it violates my expectations.

  17. feross commented on Mar 20, 2015

    @feross
    feross
    on Mar 20, 2015
    ContributorAuthor

    Breaking working npm packages is not cool, even if it's the right API decision for iojs.

    If we're going to make an API more restrictive in the inputs it accepts, we should strive to have a deprecation notice for a while first. As Rod says, it's the gentler way to proceed and nicer to users :)

  18. cjihrig
    added a commit that references this issue on Mar 20, 2015

    path: reduce type checking on some methods

    8de78e4

  19. cjihrig commented on Mar 20, 2015

    @cjihrig
    cjihrig
    on Mar 20, 2015
    Contributor

    Fixed in 8de78e4

  20. cjihrig
    closed this as completedon Mar 20, 2015

  21. cjihrig commented on Mar 20, 2015

    @cjihrig
    cjihrig
    on Mar 20, 2015
    Contributor

    Let me know if you're still having problems.

  22. feross commented on Mar 20, 2015

    @feross
    feross
    on Mar 20, 2015
    ContributorAuthor

    Thanks!

  23. feross
    added a commit that references this issue on Mar 20, 2015

    fix iojs 1.6.0 issue

    2a0500d

  24. xaka commented on Mar 20, 2015

    @xaka
    xaka
    on Mar 20, 2015

    What you're saying is right, it's just it doesn't apply to this situation
    IMHO. Have you ever seen language APIs coercing NULLs and VOIDs to
    different types with different values? I personally haven't and would
    consider it as an awful side effect. Such things cost a lot of money due to
    longer debugging time. I think having a warning is the best (and must have)
    option here, and it should go away in next releases.

    On Thu, Mar 19, 2015 at 6:04 PM, Domenic Denicola notifications@github.com
    wrote:

    FWIW all string-accepting web APIs and built-in ES APIs to ToString on
    their arguments, instead of type-testing. I am surprised io.js is trying to
    break with that; it violates my expectations.

    —
    Reply to this email directly or view it on GitHub
    #1215 (comment).

  25. domenic commented on Mar 20, 2015

    @domenic
    domenic
    on Mar 20, 2015
    Contributor

    I have seen such a language, it's called JavaScript. It's very nice and much code depends on it.

  26. silverwind commented on Mar 20, 2015

    @silverwind
    silverwind
    on Mar 20, 2015
    Contributor

    We still ❤️ Brendan for this lovely coercion feature.

  27. stelcheck commented on Mar 20, 2015

    @stelcheck
    stelcheck
    on Mar 20, 2015

    Just a wild thought, but why not turn a bug into a feature?

    var folder = 'abc'
path.dirname(['some', folder, 'path'])
// some/abc/path

    The currently reported case would remain valid, and array with more than one value would now also work. Thoughts? Stupid or a good idea?

  28. RnbWd commented on Mar 20, 2015

    @RnbWd
    RnbWd
    on Mar 20, 2015

    @domenic and @feross make good points - breaking compatibility with existing modules should only happen in extreme cases. However, using path.dirname(undefined) makes no sense and is probably a programming error. I see no harm in warning people about it (without any intention of changing the API).

  29. rauchg commented on Mar 20, 2015

    @rauchg
    rauchg
    on Mar 20, 2015
    Contributor

    iojs shouldn't break existing code this easily.

    It'd be better to provide type annotations to avoid such usage at compile time (eg: Flow, TypeScript) moving forward. This is also a more elegant way to trigger warnings of deprecation than console.warns (which can introduce subtle breakage by making hot codepaths slow).

    And of course, most importantly, it wouldn't break existing code.

  30. aredridel commented on Mar 20, 2015

    @aredridel
    aredridel
    on Mar 20, 2015
    Contributor

    krakenjs/localizr was broken by this too, surprisingly!

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

    confirmed-bugIssues and PRs for confirmed bugs.pathIssues and PRs related to the path subsystem.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions