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.

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

Can't load file #29

Closed
tlkiong opened this issue Mar 23, 2016 · 27 comments
Closed

Can't load file #29

tlkiong opened this issue Mar 23, 2016 · 27 comments

Comments

@tlkiong
Copy link

tlkiong commented Mar 23, 2016

Receive the following error:

Error: Unable to load file (fail): the_file_name_that_is_passed_in.svg

@tlkiong
Copy link
Author

tlkiong commented Mar 23, 2016

This issue is fixed on the pull request #30

To fix this, go to:

/svgexport/render.js

line 43:
page.open(svgfile, function(status) {

Change that to:
page.open('file:///'+svgfile, function(status) {

However, it seems Travis CI is failing. All I did was add a few characters into the code yet the error its throwing is no environment variables check and etc.

@shakiba
Copy link
Collaborator

shakiba commented Mar 23, 2016

Can you please share the exact code or command you are using? Meanwhile I t don't think this is good solution, because for example it does not work with relative locations or any protocol other than file.

@tlkiong
Copy link
Author

tlkiong commented Mar 24, 2016

The exact command I used was:

svgexport abc.svg xxx.jpg

My OS is Windows 7. Further debug was that the protocol was unknown for "c" or "d" (depending on which drive the file is from) for abc.svg

@shakiba
Copy link
Collaborator

shakiba commented Mar 24, 2016

Can I ask where the abc.svg file is located? Probably you can just use c:\abc.svg or .\abc.svg depending on where your file is located.

@tlkiong
Copy link
Author

tlkiong commented Mar 24, 2016

The error shown:

Unable to load file (fail): C:/....../abc.svg

I have verified twice, thrice, 4 times that the path is correct.
I have also checked with phantomjs's webpage module to return statuses (errors, success and etc) during the page.open.

The error returned was:

Invalid protocol 'c'

@shakiba
Copy link
Collaborator

shakiba commented Mar 24, 2016

Have you tried backslash \ instead of forward slash /? That is c:\ (not c:/).

@tlkiong
Copy link
Author

tlkiong commented Mar 24, 2016

Seriously?
I just show you the command I use

svgexport abc.svg xxx.jpg

Clearly I didn't state the path. Clearly the path was inferred by your code (_dirname). It's not my choice it's a forward or backward slash.

Furthermore, windows by default is that format.

@tlkiong
Copy link
Author

tlkiong commented Mar 24, 2016

The reason I chose to put 'file:///' is because that's the only protocol that is supporting opening of files from the file system by a browser. (Try opening a HTML, svg or any file in your browser. Then see the url. It'll be prefix with that 'file:///')

Your concern on whether it's a relative path or not is not really valid as I placed that code at the final section (load said file in phantomjs). All relative paths have their full paths inferred before that. The proof is that I pass in a relative path of my svg and it can still work.

@shakiba
Copy link
Collaborator

shakiba commented Mar 24, 2016

As far as I know Windows uses backslash not forward slash, that is c:\.

I'm not sure why your path is not being resolved correctly, but I think it's better to fix it first.

@tlkiong
Copy link
Author

tlkiong commented Mar 24, 2016

Ouch. My bad. Sorry for that. My path is in the forward slash. And I dare say it has nothing to do with the path. If the path is wrong it will fail much earlier

@shakiba
Copy link
Collaborator

shakiba commented Mar 24, 2016

I see, so is there still any issue?

@tlkiong
Copy link
Author

tlkiong commented Mar 24, 2016

Like i have been saying from the start, the path is NOT the issue here... The issue is with the latest upgrade of phantomjs that is reading 'c' (if the file path starts with (C:)) is an invalid protocol. The path is fine. It just fails when it tries to load the path into phantomjs.

@vitalymak
Copy link

On OSX 10.11.4

Error: Unable to load file (fail): /full/path/app-icon

app-icon without .svg extension. If I rename to /full/path/app-icon.svg then no such error.

@Tiliavir
Copy link

Tiliavir commented Apr 8, 2016

Same here. If I try to execute svgexport, the path is resolved perfectly fine, but it fails to open with said error:

PS R:\GitHub\RepoName\Documentation> svgexport test.svg test.jpg 80%
Error: Unable to load file (fail): R:\GitHub\RepoName\Documentation\test.svg

OS is Windows 10; svgexport executed in PowerShell.

@tlkiong
Copy link
Author

tlkiong commented Apr 8, 2016

@Tiliavir follow my fix as I have shown above for now as the PR is not approved.

@Tiliavir
Copy link

Thanks worked perfectly - hope to see it in the repo soon...

@ArchCodeMonkey
Copy link

I was having the same problem and I have a potential fix based on the one suggested by @tlkiong but should also satisfy the concerns of @shakiba

I found that neither relative nor absolute paths would work.
error1
error2

I modifed render.js to output more information on the error.

page.onResourceError = function(resourceError) {
   console.log('Unable to load resource (URL:' + resourceError.url + ')');
   console.log('Error code: ' + resourceError.errorCode);
   console.log('Description: ' + resourceError.errorString);
};

This error information indicated that @tlkiong is on the right track as PhantomJS is confused by the URI that is being given to its open() method. I suspect this could be an issue confined to Windows systems due to the drive letter looking like a URI protocol scheme.
error3

Out of interest I checked using a HTTP URI and got the same error.
error4

Based on this information I made the following change to index.js where it is building the command list.
Original Code

    input[0] = path.resolve(cwd, input[0]);

Modified Code

var url = require('url');
var parsedUrl = url.parse(input[0]);

if (parsedUrl.protocol !== 'http:') {
   input[0] = 'file:///' + path.resolve(cwd, input[0]);
}

This fix should allow the program to work with relative and absolute file paths and also open HTTP URIs.
Rerunning the tests now successfully generates the output file.
fixed1

fixed3

fixed2

@tlkiong
Copy link
Author

tlkiong commented May 1, 2016

That was what I was thinking initially. However, at the beginning of the code where the call is made to the relative or absolute path, it is only fetching from the filesystem. This means it isn't a http URI but only can be a fs path. That's the reason for not checking whether it's a http or not as it is unnecessary.

That's my opinion though

@shakiba
Copy link
Collaborator

shakiba commented May 1, 2016

I guess @tlkiong is right, currently only reading from file is supports anyway. But index.js is a better place to do the modification. I would be happy to merge it if anyone submit a PR.

This issue is also submitted to phantomjs.

@tlkiong
Copy link
Author

tlkiong commented May 1, 2016

The reason my PR is doing it at the final stage is to give it the code a bit of flexibility in terms of being able to do anything with the file (since we need the file path) before loading it to phantomjs. If we were to do it at index.js at the start and if in the future there would be a need for new features to enable modification of file contents, it will be a problem.

@shakiba
Copy link
Collaborator

shakiba commented May 1, 2016

I see, actually I expect it to be fixed by phantomjs in future. As a short-term workaround, I think fixing it at the same place that file path is resolved would be better.

@tlkiong
Copy link
Author

tlkiong commented May 1, 2016

I don't think there will be a "fix" by phantomjs as this is the correct way for phantomjs to handle it

It was a deliberate improvement on their part to add this in

@ArchCodeMonkey
Copy link

I only suggested making a change in index.js for convenience as PhantomJS doesn't natively support the url module and that seemed to be a clean way to check if the path already referenced a protocol scheme. If you aren't worried about being able to load from HTTP then it doesn't really matter where the fix is done.

@shakiba
Copy link
Collaborator

shakiba commented May 1, 2016

@tlkiong How do you know that? There is already a similar open and confirmed issue.

@ArchCodeMonkey
Copy link

Although, something like this would probably work in render.js.

var link = document.createElement('a');
link.href = svgfile;

if (link.protocol !== 'http:') {
   svgfile = 'file:///' + svgfile;
}

@shakiba
Copy link
Collaborator

shakiba commented May 1, 2016

I added following temporary fix, can you please check it:

    if (/^[a-z]\:\\/.test(input[0])) {
      input[0] = 'file:///' + input[0];
    }

@shakiba
Copy link
Collaborator

shakiba commented May 3, 2016

I published a new version with the fix, please let me know if the issue is not resolved.

Thanks everyone for reporting and other helps!

@shakiba shakiba closed this as completed May 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants