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

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@djcsdy
Contributor

djcsdy commented Apr 15, 2013

This changeset enables the following when running from the local filesystem (previously it only worked when running from a network resource):

URI(window.location);

Equivalently, it enables the following:

var a = document.createElement("a");
a.href = "file:///C:/foo/bar.html";
URI(a);

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.

Accept file: Location object or anchor tag as an argument to new URI().
As a side-effect of this change, the test cases will now pass when running
from a local filesystem.
@djcsdy

This comment has been minimized.

Show comment
Hide comment
@djcsdy

djcsdy Apr 15, 2013

Contributor

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, URI(window.location) would throw TypeError if window.location was a file: URI. After this change, it constructs a URI as expected.

Contributor

djcsdy commented Apr 15, 2013

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, URI(window.location) would throw TypeError if window.location was a file: URI. After this change, it constructs a URI as expected.

@rodneyrehm

This comment has been minimized.

Show comment
Hide comment
@rodneyrehm

rodneyrehm Apr 15, 2013

Member

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 <a href="…">, but what about <img src=""> and others?

Member

rodneyrehm commented Apr 15, 2013

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 <a href="…">, but what about <img src=""> and others?

@djcsdy

This comment has been minimized.

Show comment
Hide comment
@djcsdy

djcsdy Apr 15, 2013

Contributor

I don’t especially care about <a href>. The reason I put in the <a href> test is that it implements the same properties as window.location, so if URI(a) works for an <a href> with a given URI, that ought to mean that URI(window.location) will work for that same URI.

(I don’t especially care about window.location either; I’d quite happily just call URI(window.location.href), but I noticed some test cases were failing when run locally and it seemed reasonable to try to fix them).

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 window.location.hostname to the empty string. In that situation, URI(window.location) still returns a URI with hostname() === null as expected.

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

Contributor

djcsdy commented Apr 15, 2013

I don’t especially care about <a href>. The reason I put in the <a href> test is that it implements the same properties as window.location, so if URI(a) works for an <a href> with a given URI, that ought to mean that URI(window.location) will work for that same URI.

(I don’t especially care about window.location either; I’d quite happily just call URI(window.location.href), but I noticed some test cases were failing when run locally and it seemed reasonable to try to fix them).

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 window.location.hostname to the empty string. In that situation, URI(window.location) still returns a URI with hostname() === null as expected.

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

@djcsdy

This comment has been minimized.

Show comment
Hide comment
@djcsdy

djcsdy Apr 15, 2013

Contributor

BTW, I’m happy to remove the <a href> stuff if you don’t agree with the reasoning above. That said, I should point out that the change doesn’t add any HTML dependency to URI.js itself, just to the test case – and the test cases are already dependent on HTML for window.location :).

Contributor

djcsdy commented Apr 15, 2013

BTW, I’m happy to remove the <a href> stuff if you don’t agree with the reasoning above. That said, I should point out that the change doesn’t add any HTML dependency to URI.js itself, just to the test case – and the test cases are already dependent on HTML for window.location :).

@rodneyrehm

This comment has been minimized.

Show comment
Hide comment
@rodneyrehm

rodneyrehm Apr 15, 2013

Member

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,
Rod

Member

rodneyrehm commented Apr 15, 2013

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,
Rod

@djcsdy

This comment has been minimized.

Show comment
Hide comment
@djcsdy

djcsdy Apr 15, 2013

Contributor

URI() already accepts <a> elements as a side-effect of the fact that it accepts Location objects (like window.location). This changeset doesn’t change anything in that regard. The only reason I even mentioned <a> elements is because they’re useful for simulating Location objects with different URIs.

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.

Contributor

djcsdy commented Apr 15, 2013

URI() already accepts <a> elements as a side-effect of the fact that it accepts Location objects (like window.location). This changeset doesn’t change anything in that regard. The only reason I even mentioned <a> elements is because they’re useful for simulating Location objects with different URIs.

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.

@rodneyrehm

This comment has been minimized.

Show comment
Hide comment
@rodneyrehm

rodneyrehm Apr 15, 2013

Member

Fair enough! thanks for pointing things out though. Much appreciated :)

Member

rodneyrehm commented Apr 15, 2013

Fair enough! thanks for pointing things out though. Much appreciated :)

@rodneyrehm

This comment has been minimized.

Show comment
Hide comment
@rodneyrehm

rodneyrehm Aug 3, 2013

Member

I've merged this into master and enhanced it a bit - it will be included in the next release. thank you for your help!

Member

rodneyrehm commented Aug 3, 2013

I've merged this into master and enhanced it a bit - it will be included in the next release. thank you for your help!

@rodneyrehm rodneyrehm closed this Aug 3, 2013

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