alanshaw / markdown-pdf

:page_facing_up: Markdown to PDF converter
https://npmjs.org/package/markdown-pdf
MIT License
2.7k stars 253 forks source link

image in output #65

Open sander76 opened 9 years ago

sander76 commented 9 years ago

Using this from the command line. But I am not able to insert an image in the output. Regardless how I reference it. (absolute vs relative path..) Anyone any hints ?

anko commented 9 years ago

If I have

doc.md:

![optional image description](image.png)

and an image.png in the same directory, then run markdown-pdf doc.md, I get a doc.pdf containing the image.

Could you post an SSCCE of your attempt that didn't work?

sander76 commented 9 years ago

Running windows. folder c:\temp\pdf-test\ folder contains app.md and remote-front.png

app.md:

![test](remote-front.png)

running markdown-pdf app.md doesn't result in an image output.

Found out when doing:

![test](c:/temp/pdf-test/remote-front.png)

results in correct output.. But I want to use a relative path.... I guess node is using a working directory I cannot change (or see). when running:

markdown-pdf -c "c:/temp/pdf-test"  app.md

doesn't seem to do anything. Is there some log stating which path markdown-pdf is using ?

bengreenier commented 8 years ago

+1 same issue - any updates on this?

anko commented 8 years ago

I don't have a Windows machine to test on, but doing similar steps as https://github.com/alanshaw/markdown-pdf/issues/65#issuecomment-147191310 works on Linux (see below), so I suspect there's some oddness going on with how the given path is resolved.

Could someone on Windows check what opts.cwd is before and after this line in index.js—say with a console.log call when doing these steps?

If you've installed the program in the current directory using npm install markdown-pdf (i.e. without using -g), you should find the file in node_modules/markdown-pdf/index.js, and can run the program using node node_modules/.bin/markdown-pdf <arguments>.


Here's how I tested on Linux when everything works:

I put my markdown file in /home/an/test.md, my image in /home/an/test/image.png and made the image path in the markdown ![](image.png) a relative link.

I then edited /home/an/test/node_modules/markdown-pdf/index.js equivalently to this diff:

*** test/node_modules/markdown-pdf/index.js 2016-01-21 21:58:34.720571257 +0100
--- test/node_modules/markdown-pdf/index.js 2016-01-21 22:03:14.538047193 +0100
***************
*** 15,17 ****
--- 15,19 ----
    opts = opts || {}
+   console.log("BEFORE", opts.cwd);
    opts.cwd = opts.cwd ? path.resolve(opts.cwd) : process.cwd()
+   console.log("AFTER ", opts.cwd);
    opts.phantomPath = opts.phantomPath || require("phantomjs").path

I then ran this command:

node ./test/node_modules/.bin/markdown-pdf -c "/home/an/test" test.md

And got a /home/an/test.pdf with the correct image in it, and this output:

BEFORE /home/an/test
AFTER  /home/an/test
BEFORE /home/an/test
AFTER  /home/an/test

The markdownpdf function seems to have run twice internally, both times having gotten the correct path already before the path.resolve-line.

alanshaw commented 8 years ago

@sander76 @bengreenier did you resolve your problem? If not perhaps try the latest version of markdown-pdf (7.0.0) which is using the new version of phantomjs. Let me know how you get on.

bpiepiora commented 8 years ago

The problem seems to be the output from Remarkable. Changing the following code in markdown-pdf/index.js solves the issue for me on Windows.

From:

...
    try {
      mdParser.inline.ruler.enable([rule])
    } catch (err) {}
  })

  self.push(mdParser.render(md))
  self.push(null)

To:

...
    try {
      mdParser.inline.ruler.enable([rule])
    } catch (err) {}
  })

  var htmlCode = mdParser.render(md).replace('<img src="', '<img src="file:///');

  self.push(htmlCode)
  self.push(null)

The missing file:/// prevents phantomjs from reading the image files correctly. You also have to fix code in markdown-pdf/phantom/render.js to get CSS working properly:

// Resources don't load in windows with file protocol
var protocol = os.name === 'windows' ? 'file:///' : 'file://'

Warning: This is no clean solution and works only if the image files are in the same directory as the markdown files.

anko commented 8 years ago

@bpiepiora Thanks for investigating. That sounds like a great starting point for a full fix.

Idea: On Windows, trumpet-transform <img> tags in the HTML to prepend file:/// to their src.

snaga commented 8 years ago

Is this issue still in the code? I got the same issue on Windows when installing using npm.

evil-shrike commented 8 years ago

I also have the issue of missing images on Windows in v7.0.0. Is there a version with fix somewhere?

jurgenhaas commented 7 years ago

Same problem here. Even images in running.js are not being output.

jurgenhaas commented 7 years ago

I have tried the change in https://github.com/alanshaw/markdown-pdf/issues/65#issuecomment-194237109 which works gret for images in the markdown file.

However, when I want to include an image in the running.js for the header, it doesn't work and I tried debugging a lot, without any luck. Any idea where I could get help from?

ezze commented 7 years ago

In order to fix images' URLs you don't need any fixes in the source code. Just use preProcessHtml option.

My solution is based on @nickcmaynard's code and @bpiepiora's comment (you need to npm install through2 and npm install cheerio).

Tested with markdown-pdf version 7.0.0 and Windows 7 64-bit:

var path = require('path'),
    through = require('through2'),
    cheerio = require('cheerio'),
    markdownPdf = require('markdown-pdf');

var preProcessHtml = function(basePath) {
    return function() {
        return through(function(chunk, encoding, callback) {
            var $ = cheerio.load(chunk);

            $('img[src]').each(function() {
                var imagePath = $(this).attr('src');
                imagePath = path.resolve(basePath, imagePath);
                $(this).attr('src', 'file://' + (process.platform === 'win32' ? '/' : '') + imagePath);
            });

            this.push($.html());
            callback();
        });
    }
};

var basePath = path.resolve(__dirname, 'markdown-source');
markdownPdf({
    preProcessHtml: preProcessHtml(basePath)
}).concat.from([
   // ... list of markdown files
]).to(path.resolve(__dirname, 'out.pdf'), function() {
    console.log('PDF is created.');
});

I didn't investigate the problem with CSS links due to it's not required for me yet.

purtuga commented 7 years ago

I can confirm that this is still happening for me... the cwd seems to be correctly set in index.js as to what I define it on input (btw: what is cwd? and how is it used or how does it impact the output? Docs are no clear).

Relative paths for images in markdown files do not seem to be rendered on Windows when using the file:/// approach to show a webpage in the browser. They do show up (as @sander76 stated) if you use absolute paths, but that is just not a good option. My quick fix was to take the above mentioned work-around (to replace file:// with file:/// but also add on the opts.cwd, which I have set to be the root folder containing my MD files. Something like:

var htmlCode = mdParser.render(md).replace('<img src="', '<img src="file:///' + opts.cwd + "/");
self.push(htmlCode)

I also tried to play with the "base" (<base>) tag in html5bp/index.html and set that to the root folder that contained my md files, which did work only if I removed the "file://" from images.

My setup up:

project/
    node_modules/
    bin/
        create-about.js
    docs/
        images/
            image1.png
        about.md
        versions.md

Content of about.md:

Some image:
![image 1](image1)

[image1]: ./images/image1.png

Command that was run (my own script called bin/create-readme.js:

markdownPdf({ cwd: projectDocsFolder })
    .concat.from([
        path.resolve(projectDocsFolder, "about.md"),
        path.resolve(projectDocsFolder, "versions.md")
    ])
    .to(myoutputfile);

/Paul

anko commented 7 years ago

I'd like to help here, but I don't have a Windows machine to test on. I tried creating a Windows build on AppVeyor in this branch a while ago (so we'd have a common reference point that this works on Windows), but I got stuck. Maybe someone knows how to continue from there?


@purtuga

btw: what is cwd? and how is it used or how does it impact the output? Docs are no clear

Just for reference, cwd stands for "current working directory". It's the path that PhantomJS is run in, so PhantomJS resolves any directory paths mentioned in pages relative to that directory.

purtuga commented 7 years ago

Thanks for the response @anko ... I did know what cwd meant, but was not sure how it was being used in this library... So it controls what "base directory" to where phantomJS executes the webpage from... Thanks for that... I thought that was perhaps the way it was being used.

I will also try to fork this problem and work on it a little more to see if I can find a solution...

Have you thought about perhaps running a webserver to host the generated html? and then having phomtomJS load that instead? This approach could avoid all of the issues around OS/platform.

Btw: regarding windows machine... Not sure if MS restricts the use of these only to the browser, but they do have VMs you can run on virtualbox for browser testing... :) Use it for IE testing, and never really tried to install nodeJS, etc. on it...

https://developer.microsoft.com/en-us/microsoft-edge/tools/vms/

remcoros commented 7 years ago

Changing

var protocol = os.name === 'windows' ? '' : 'file://'

to

var protocol = os.name === 'windows' ? 'file:///' : 'file://'

worked for me on Windows, also for relative paths.

purtuga commented 7 years ago

I submitted a proposed solution based on the topic/issues discussed in this issue. To try it out, do the following:

npm install purtuga/markdown-pdf#master

and provide feedback to whether it works.

HoverBaum commented 7 years ago

@purtuga using that refering to an image as image.jpg or ./image.jpg both works fine for me and generates a PDF with the image in it.

remcoros commented 7 years ago

Works for me too (I did this already with a local patch). Love to see this merged back in.