Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Url accessor fixes #1589

Closed
wants to merge 2 commits into from
Closed

Url accessor fixes #1589

wants to merge 2 commits into from

Conversation

petkaantonov
Copy link
Contributor

See #1588

Also includes a commit that fixes href cache invalidation bug as described in #1588 (comment)

cc: @silverwind @domenic @rvagg @chrisdickinson

@petkaantonov
url: delete href cache on all setter code paths
7702b0d
@petkaantonov
url: fix treatment of some values as non-empty
e274b92 
In addition to null, undefined and the empty string
are treated as empty (removing the component from the url).

The string '#' is treated same as empty values when
setting .hash.

The string '?' is treated same as empty values when
setting .search.

Fixes nodejs#1588
@petkaantonov
Copy link
Contributor Author

Can someone test with restify? All tests pass now with request. I cannot get restify to work at all

@silverwind
Copy link
Contributor

I'll compile and test both again.

@bnoordhuis
Copy link
Member

Uninformed LGTM with a question.

@silverwind
Copy link
Contributor

request and restify install fine, except I get the usual gyp 404 with nightly builds, which is unrelated.

@silverwind
Copy link
Contributor

The href cache updating looks fixed too.

@Fishrock123
Copy link
Contributor

With this patch applied make test-npm still appears to fail? https://gist.github.com/Fishrock123/bfbc8e873c53a144e169

@petkaantonov
Copy link
Contributor Author

make test-npm seems to use installed io.js, not the one you just built. From your output it really appears that you are just testing with the 2.0RC, not with this patch as it should not be even possible to have an url with empty hash now.

@silverwind
Copy link
Contributor

Unrelated to this PR, but notices this npm test failure:

Command: "/usr/local/bin/iojs config-credentials.js"

        found:  |
          https://registry.lvh.me:8661/
        wanted: |
          //registry.lvh.me:8661/

test: https://github.com/npm/npm/blob/master/test/tap/config-credentials.js
uses: https://github.com/npm/npm/blob/master/lib/config/clear-credentials-by-uri.js
which uses: https://github.com/npm/npm/blob/master/lib/config/nerf-dart.js

The final file is of interest, it deletes a lot of url properties.

@silverwind
Copy link
Contributor

opened #1591 for the delete issue.

@silverwind
Copy link
Contributor

Also, LGTM on this patch.

@silverwind silverwind mentioned this pull request May 2, 2015
Closed
@mscdex mscdex added the url Issues and PRs related to the legacy built-in url module. label May 2, 2015
petkaantonov added a commit that referenced this pull request May 3, 2015
@petkaantonov
url: delete href cache on all setter code paths
dbdd81a 
PR-URL: #1589
Fixes: #1588
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
petkaantonov added a commit that referenced this pull request May 3, 2015
@petkaantonov
url: fix treatment of some values as non-empty
6687721 
In addition to null, undefined and the empty string
are treated as empty (removing the component from the url).

The string '#' is treated same as empty values when
setting .hash.

The string '?' is treated same as empty values when
setting .search.

PR-URL: #1589
Fixes: #1588
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@petkaantonov
Copy link
Contributor Author

Landed in 6687721

@petkaantonov petkaantonov deleted the url-accessor-fixes branch May 3, 2015 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@petkaantonov @silverwind @bnoordhuis @Fishrock123 @mscdex