piqnt / svgexport

SVG to PNG/JPEG command-line tool and Node.js module
927 stars 85 forks source link

Can't load file #29

Closed tlkiong closed 8 years ago

tlkiong commented 8 years ago

Receive the following error:

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

tlkiong commented 8 years ago

This issue is fixed on the pull request https://github.com/shakiba/svgexport/pull/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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

tlkiong commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

I see, so is there still any issue?

tlkiong commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

Tiliavir commented 8 years ago

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

ArchCodeMonkey commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

ArchCodeMonkey commented 8 years ago

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 commented 8 years ago

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

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

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!