Skip to content
This repository has been archived by the owner on Dec 16, 2023. It is now read-only.

Fix file path in test case. #886

Merged
merged 2 commits into from
Mar 26, 2015
Merged

Fix file path in test case. #886

merged 2 commits into from
Mar 26, 2015

Conversation

jagoda
Copy link
Contributor

@jagoda jagoda commented Mar 25, 2015

This test case was broken on Linux running from a path containing an upper case letter. I may be missing something, but I'm not sure why it would be desirable to convert the path to lower case on any platform.

@assaf
Copy link
Owner

assaf commented Mar 25, 2015

Because Node in its infinite wisdom mangles file: URLs and down-cases the root directory name, which on the Mac happens to be /Users.

@jagoda
Copy link
Contributor Author

jagoda commented Mar 26, 2015

Ah, interesting. I dug into this some more. I believe that Node is almost doing the right thing. It seems that file URLs technically have a hostname component (even though it is basically ignored). This means that a URL of the form file://User/foo should treat User as the hostname and not as part of the path. Node's URL.parse() lowercases all hostnames. Browsers like Chrome and Firefox on the other hand seem to deal with this by simply ignoring or dropping the hostname value for file URLs (respectively).

The problem seems to manifest in Node's URL.resolve(). Node seems to have a bug that causes the first path element to be resolved to the hostname when the source protocol is not file: but the target protocol is. Zombie seems to get bit by this in browser.visit() when it calls URL.resolve().

I suppose that the solution is ultimately to try and work this through the Node community. However, I was wondering what you think about trying to put together a work-around for Zombie. I would be happy to take a crack at it if it sounds good to you.

@assaf
Copy link
Owner

assaf commented Mar 26, 2015

The test cases actually passes file:///User/ to Node, which is a correct URL with empty hostname, and path that starts with /User. It's Node that does weird things afterwards.

If you have an idea how to fix this in Zombie, and want to add it to this PR, I'll gladly accept both changes together.

@jagoda
Copy link
Contributor Author

jagoda commented Mar 26, 2015

The test cases actually passes file:///User/ to Node, which is a correct URL with empty hostname, and path that starts with /User. It's Node that does weird things afterwards.

Agreed. I believe it's the URL.resolve() call in browser.visit() that is the culprit. My lengthy explanation was probably a bit much, but I wanted to provide some background for where the strange lowercasing came from in the Node code. In any case, I think I can work something out.

@jagoda
Copy link
Contributor Author

jagoda commented Mar 26, 2015

Upstream issue is nodejs/node-v0.x-archive#14146. Going to try to put together a work around for Zombie now.

@jagoda
Copy link
Contributor Author

jagoda commented Mar 26, 2015

I think this should work around the issue. Let me know what you think.

@assaf
Copy link
Owner

assaf commented Mar 26, 2015

When we have a document, we use resourceLoader.resolve, which effectively uses this algorithm: https://github.com/tmpvar/jsdom/blob/master/lib/jsdom/utils.js#L177

I think we should just use the JSDOM resolver everywhere and not bother with URL.resolve.

@jagoda jagoda force-pushed the fix-file-system branch 2 times, most recently from a972ff4 to 94ca412 Compare March 26, 2015 17:36
@jagoda
Copy link
Contributor Author

jagoda commented Mar 26, 2015

Cool. Switching to the JSDOM resolver seems reasonable since it seems to address other bugs. However, it doesn't seem to address the root issue here. I have updated everything to use Utils.resolveHref() and have added the bug fix to the JSDOM patches. Thoughts?

@jagoda
Copy link
Contributor Author

jagoda commented Mar 26, 2015

It seems that the JSDOM is no longer supported on Node (>= 4.0). What are your thoughts going forward with new features?

@assaf
Copy link
Owner

assaf commented Mar 26, 2015

I'm also going to drop Node support at some point, it takes too much effort to test and patch the code against multiple runtimes, stuff like this: assaf/node-replay#60

Zombie 3.1 will remain with JSDOM 3.1, but the next major upgrade will drop 0.10 support and, if JSDOM doesn't come around, also 0.12 support.

@jagoda
Copy link
Contributor Author

jagoda commented Mar 26, 2015

Also submitted the upstream fix to nodejs/node#1277. Any thoughts on the latest changes?

assaf added a commit that referenced this pull request Mar 26, 2015
@assaf assaf merged commit 651f7f3 into assaf:master Mar 26, 2015
@assaf
Copy link
Owner

assaf commented Mar 26, 2015

Fantastic!

@assaf
Copy link
Owner

assaf commented Mar 26, 2015

Just published 3.1.1

@jagoda
Copy link
Contributor Author

jagoda commented Mar 27, 2015

Awesome, thanks!

@jagoda jagoda deleted the fix-file-system branch March 27, 2015 00:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants