-
Notifications
You must be signed in to change notification settings - Fork 476
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.
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
Accept Location object or anchor element representing a filesystem URL as an argument to new URI(). #77
Conversation
As a side-effect of this change, the test cases will now pass when running from a local filesystem.
I just read this pull request back and I don’t think I could have obfuscated the description more if I’d tried. Sorry! Before this change, |
While accept that we have a general problem here, I don't like the solution all that much. If no hostname is available, it should be null (like any other not set parts). I didn't make URI.js (core) touch HTML because I put that into the jQuery plugin. Anyway, you're only looking at |
I don’t especially care about (I don’t especially care about As for the hostname, either you’re misreading the change I made or I’m misunderstanding you :). I just modified the test to tolerate the fact that, for files on the local filesystem, the browser sets I could have done a better job of explaining my reasoning in the first place. I was being distracted by colleagues while I was writing the commit log and pull request, sorry! :) |
BTW, I’m happy to remove the |
Ok, I think I need to clarify… I'm not entirely against accepting DOMElements (although I do have mixed feelings about this). I'm just saying if it's done, it has to be done properly. Same thing goes for the local file system access, if it's done, it's got to be done properly - that means (if necessary - manually) resetting certain properties. I'm going to leave this PR open for the next round… If you can spare the time, feel free to update the PR, otherwise I'll look what I can make of it once I get back to it… (some time in May, I guess) Cheers, |
For local filesystem URLs, I don’t know what you’re getting at and I don’t actually need that functionality – I just thought it would be nice if the tests passed – so I’ll leave it at that. |
Fair enough! thanks for pointing things out though. Much appreciated :) |
I've merged this into master and enhanced it a bit - it will be included in the next release. thank you for your help! |
This changeset enables the following when running from the local filesystem (previously it only worked when running from a network resource):
Equivalently, it enables the following:
As a side-effect, this changeset fixes the unit tests when running from the local filesystem.
Prior to this change, when running from the local filesystem, tests would fail as follows:
constructing: new URI(object)
:TypeError: invalid input at URI.p.href (URI.js:847:15)
constructing: new URI()
: fail:hostname == location.hostname
I’ve also added some new tests to verify the new features.