Prevent default link behaviour when hashbang: true and protocol is file:

[kaisermann]

Fix for #492

[coveralls]

Coverage decreased (-0.3%) to 92.116% when pulling 1546f06101ba9ec0aae1e3cc0dd692dd92893f5e on kaisermann:fix-hashbang-file into ec2bba421178f1b269dc09e75fb540cc8893df95 on visionmedia:master.

[PaulMaly]

@matthewp Can we merge this PR to the new release as soon as it's possible?

[matthewp]

Need to get the code coverage passing first.

[kaisermann]

I've added a basic test for this behavior but I couldn't get it passing because the href=""s in test-page.html start with a ./, not a / and that seems to fail in this case. If I change the contact href from ./contact to /contact it works as expected but fails everywhere else.

[PaulMaly]

@matthewp Can we somehow skip it?

[kaisermann]

I think the main problem with that test is because ./contact results in a pathname of /var/www/contact, not /contact as it should. I've pushed a commit that changes a little bit the _getBase to get the dirname instead of the full path when protocol is file:.

[matthewp]

Skipping tests won't help with the code coverage will it?

[kaisermann]

I'm wrapping my head around on how to fix the coverage and I don't have much free time right now. @PaulMaly or @matthewp Can one of you guys help me with this :grin:? Otherwise, just give me some days!

[matthewp]

There might be something weird going on because other PRs are failing coverage too. If you'll just get the tests passing again I'll skip the coverage for now.

[kaisermann]

It is passing since 3a75d238d8d486876bbbdc26275ccee021a89d6d, but I don't undestand why it's failing on node 0.1:

I'm gonna try to fix this if it's possible

[kaisermann]

@matthewp I've managed to reproduce the failing test for node 0.1 but couldn't find an exact reason because it works fine for node 6+. Could it be related to an old version of jsdom? Node 6+ seems to install version 11.12.0 while Node 0.1 installs 1.5.0.

[PaulMaly]

@matthewp Maybe we can skip failing tests for this PR?

[kaisermann]

@PaulMaly The tests are passing now (if you disconsider node 0.1 :grin:)

[PaulMaly]

@kaisermann awesome, but it’s still not in release ((

[matthewp]

We can't skip failing tests. We can skip the code coverage since I think there is something weird happening there. If you branch straight off of master does the old Node version work?

[kaisermann]

Yes it works on master, but it doesn't have the test for resolving a clicked <a> when protocol is file://.

I was trying to debug the jsdom 1.3.1 and it seems it doesn't resolve the URL correctly:

> jshint index.js test/tests.js && mocha test/tests.js

  File protocol
Resolving <a> HREF: about:blank
Base Url: http://example.com
Href:  about:blank
JSDOM Intermediate:  about:blank

>> Final resolved href:  about:blank

Resolving <a> HREF: ./contact
Base Url: about:blank
Href:  ./contact
JSDOM Intermediate:  about:contact

>> Final resolved href:  about:contact

Resolving <a> HREF: ./contact
Resolving <a> HREF: about:contact
Base Url: about:blank
Href:  about:contact
JSDOM Intermediate:  about:contact

>> Final resolved href:  about:contact

Resolving <a> HREF: about:contact

All these logs are from the last test in test.js. I've put them inside the jsdom resolve method. Maybe it's a configuration issue since there's those about:blank paths.

Anyway, the failing test for node 0.1 seems to be caused by jsdom resolving the ./contact path to about:contact.

Related jsdom resolve code: https://github.com/jsdom/jsdom/blob/b41f52f51cebc8884f818d54636da78e451f5c5f/lib/jsdom/level2/html.js#L28

[matthewp]

Yeah, so it's because of your added test. So let's do this: remove that test. We'll ignore codecoverage and get this in.

[kaisermann]

@matthewp Done! Thanks for your patience @matthewp :grin:

[matthewp]

This was released in 1.11.0