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

Integrate mdpdf as the primary converter #162

Closed
travs opened this issue Jul 26, 2017 · 22 comments
Closed

Integrate mdpdf as the primary converter #162

travs opened this issue Jul 26, 2017 · 22 comments

Comments

@travs
Copy link
Owner

travs commented Jul 26, 2017

Changing because the output of mdpdf looks so great.
Finally now getting around to starting this.

I have one commit pushed to this branch, but am getting an error regarding atom's content security policy, like this:

Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'self'".


    at Object.createFunctionContext (/home/travis/github/markdown-pdf/node_modules/handlebars/dist/cjs/handlebars/compiler/javascript-compiler.js:254:23)
    at Object.compile (/home/travis/github/markdown-pdf/node_modules/handlebars/dist/cjs/handlebars/compiler/javascript-compiler.js:142:19)
    at compileInput (/home/travis/github/markdown-pdf/node_modules/handlebars/dist/cjs/handlebars/compiler/compiler.js:517:53)
    at ret (/home/travis/github/markdown-pdf/node_modules/handlebars/dist/cjs/handlebars/compiler/compiler.js:524:18)
    at readFile.then.then.then.then.md (/home/travis/github/markdown-pdf/node_modules/mdpdf/index.js:161:16)
    at tryCatcher (/home/travis/github/markdown-pdf/node_modules/bluebird/js/release/util.js:16:23)
    at Promise._settlePromiseFromHandler (/home/travis/github/markdown-pdf/node_modules/bluebird/js/release/promise.js:512:31)
    at Promise._settlePromise (/home/travis/github/markdown-pdf/node_modules/bluebird/js/release/promise.js:569:18)
    at Promise._settlePromise0 (/home/travis/github/markdown-pdf/node_modules/bluebird/js/release/promise.js:614:10)
    at Promise._settlePromises (/home/travis/github/markdown-pdf/node_modules/bluebird/js/release/promise.js:693:18)
    at Promise._fulfill (/home/travis/github/markdown-pdf/node_modules/bluebird/js/release/promise.js:638:18)
    at /home/travis/github/markdown-pdf/node_modules/bluebird/js/release/nodeback.js:42:21
    at tryToString (fs.js:455:3)
    at FSReqWrap.readFileAfterClose [as oncomplete] (fs.js:442:12)

It appears that this is due to some eval call in mdpdf (most likely in one of its dependencies), but I couldn't find such a call.
I have tried wrapping the mdpdf usage in loophole to escape the CSP, but I still get the error.

I would much rather use the mdpdf API, but if I can't get this resolved then we may have to resort to calling mdpdf from the command line in a child process (not a clean solution).

@BlueHatbRit
Do you have any idea of where such a call may be in the mdpdf stack? If so, maybe I can help make a workaround that lets us use mdpdf inside atom given its current CSP.

@elliotblackburn
Copy link
Collaborator

Hi @travs, amazing to see the work you've been doing. It's great to see the work being done to try out this transition!

I've not had a chance to dig through this yet specifically but I get the feeling it'll be something to do with either PhantomJS (used to produce the final pdf) or highlightjs (used to add syntax highlighting when rendered inside of phantomjs).

I'll pull the branch and have a dig around to see what I can find but I'm not super well versed in atom or coffeescript. Would you be able to grab a sample of the options and data being pushed into the convert function so I can try and isolate out atom when testing?

@elliotblackburn
Copy link
Collaborator

On a second look this is actually being caused by a line of handlebars code, I'm just going to dig into their source now and see what's cracking but it looks to me like they're calling eval when run under nodejs.

@elliotblackburn
Copy link
Collaborator

elliotblackburn commented Jul 26, 2017

The offending line seems to be a piece of handlebars code, specifically:

return Function.apply(this, params); (as the stack trace shows) but I can't find any reference to eval in the project project other than in their tests. I honestly have no idea why this is happening right now. I'll dig into it more over the next day and see what I can find but I have a feeling this will need us to get in touch with the handlebars project team. It seems it undertakes different ways of running compilations depending on the platform so it's possible some sort of eval is being called somewhere. I just can't find reference to it in any of the distributed src for node.

@elliotblackburn
Copy link
Collaborator

Just a hunch, but instead of unsafe eval, try using the loophole unsafeNewFunction as that seems somewhat similar to the actual handlebars code that's being executed. It's a longshot but it's worth ruling out since that's actually closer to the code being run.

@travs
Copy link
Owner Author

travs commented Jul 27, 2017

Just a hunch, but instead of unsafe eval, try using the loophole unsafeNewFunction

Just tried this and getting the same error. Also tried allowUnsafeNewFunction and allowUnsafeEval together with the same effect.

For the options I just put in something minimal:

let options = {
          source: inPath,  // using README.md from this project
          destination: outPath,  // ./README.pdf
          pdf: {
              format: 'A4'
          }
        };

I couldn't find an eval in handlebars either; I hope this is not a false positive somehow, from the CSP.
However, we have these two issues from the handlebars repo, which make me think we are not alone (though perhaps out of luck with regard to their implementing a fix):

It also may be that I am using loophole incorrectly somehow, as Promises are run async and I'm not sure if loophole supports this. This guy seems to have gotten it to work with his Promises (Q), so I may give this a try soon if all else fails.

I'm not 100% sure how mdpdf works inside, but is there any way we can precompile with handlebars rather than doing it at runtime?
Seems like that is impossible by its nature, but you know more about it than me!

@elliotblackburn
Copy link
Collaborator

In mdpdf I use handlebars to pull together several things into one html document.

  • The users markdown (converted into html).
  • Any headers and footers that must be placed into the correct location of the document.
  • Default and user provided stylesheets.

To get all of these into one html document I'm using handlebars almost as you might in an express application.

There are some alternatives which I'd be happy to experiment with. My first go-to would be to look at the lodash template function and see if that gives us what we need. Failing that I might checkout moustache or Pug (jade).

However this would be quite a change to the module as the majority of the code is just building that final html document.

It would be good if we could look at Q just to be sure that this isn't something loophole can fix. If not then I can take a look at a new templating engine and see what might work.

@elliotblackburn
Copy link
Collaborator

This might also be really dumb, but in the past I've seen libraries which support promises have had issues with anonymous arrow functions () => {} (mocha being one). It might be worth using function () {} just to ensure we're not losing any state through that.

@travs
Copy link
Owner Author

travs commented Aug 6, 2017

@BlueHatbRit
Hey Elliot, I have tried your last suggestion and a couple more attempts (wrapping the actual require statement for mdpdf in every combination of loophole calls), and still no success on my end.

However, I have been able to produce PDF output by wrapping some calls within mdpdf itself, like this:

const loophole = require('loophole');
// ...
const html = loophole.allowUnsafeNewFunction(function() {
  return template(local)
});

I'm not sure if you would accept this change in mdpdf or not, but if you are ok with it, then we just have one last issue to tackle.

The PDF output is, for some reason, massively oversized when I use the code above (invoked from the markdown-pdf module at the tip of the mdpdf branch).

This is what the output of this project's README looks like:
README.pdf

When I call the unmodifed mdpdf from the command line (no arguments), the output looks perfect.

Do you have any idea why this could be happening? Some stylesheet not being applied, or something?

@elliotblackburn
Copy link
Collaborator

@travs
Good to hear about the progress, it sounds really positive.

I've had a look at the loophole repo and it seems it gets published in JS (not coffee) and has no external dependencies. That means I'd be happy to look at merging that change into mdpdf until we can find a better solution. If you wanted to go ahead and submit a PR for that then I'll run a few tests to make sure it doesn't add too much overhead and then we'll get it merged and released.

That PDF output is very strange! Could you pass me an updated version of the parameter values you're using to invoke mdpdf? If the same version of the CLI is coming out fine then the only two things I can think of are:

  • The parameters being passed through to mdpdf are getting malformed somehow before entering the library (perhaps incorrect unit strings or something).
  • Atom is causing something to happen to the HTML as it's being processed.

Have you tried applying the debug flag to check the html output? That should reveal any issues as that html is fed straight into phantomjs.

@travs
Copy link
Owner Author

travs commented Aug 19, 2017

@BlueHatbRit

I have this minimal script that I run to get the messed up output:

const mdpdf = require('mdpdf');
const path = require('path');

let options = {
      source: '../markdown-pdf/README.md',
      destination: path.join(__dirname, 'output.pdf'),
      pdf: {
        format: 'A4'
      }
};

mdpdf.convert(options).then((pdfPath) => {
      console.log('PDF Path:', pdfPath);
}).catch((err) => {
      console.error(err);
});

This is actually using an unmodifed version of mdpdf (i.e. before I even add loophole).

The README.md is just the readme from this repo, and I can confirm that the CLI of the same mdpdf version (1.5.0) outputs really nice output still.

I'm really confused by this, since that script is basically the up-and-running script from the mdpdf readme.
Could it be something to do with phantom-js perhaps?
I remember there used to be some rendering differences between platforms a few years ago.
Unfortunately I don't have another system to test this on right now, otherwise it would be easy to see whether it was this 😛

For reference I am on linux (4.11.9-1-ARCH).

update

Forgot to upload the HTML output from debug.
Here is the script output, and here is the cli output.

@elliotblackburn
Copy link
Collaborator

@travs the cli output you linked at the bottom is giving me a 404, are you sure it's public?

Cheers for that code that's really helpful! I'm on macOS so we might get a difference but I'll give it a go right now. I am considering moving over to the newly released Puppeteer by the Chrome team but that's probably a way off as it'll mean quite a lot more code being introduced, just a heads up though.

I'll run this through and see what I get out.

@travs
Copy link
Owner Author

travs commented Aug 19, 2017

@BlueHatbRit

Mate, I'm consistently impressed with how quick you are.
I have the problem solved now! 😄

I was just about to update my comment again but you beat me to it.
Expect a PR on mdpdf in T-minus 2 minutes

@elliotblackburn
Copy link
Collaborator

@travs I'm impressed how you manage to catch me at the right time!

What was the issue? I'd be interested to know so I can make sure the docs are better for future users.

Looking forward to the PR, excited to get mdpdf rolled into this module!

@travs
Copy link
Owner Author

travs commented Aug 19, 2017

@BlueHatbRit Responded over here.

@elliotblackburn
Copy link
Collaborator

@travs your changes have been integrated and a release has been published to npm under 1.5.1 - https://www.npmjs.com/package/mdpdf.

The documentation changes mentioned on the PR will be sorted over the next couple of days, I pushed the release before that to help you get moving fasting.

Ping me again if there's anything you need help with for this issue, I'll keep an eye on it anyway to see how you make use of mdpdf :)

@travs
Copy link
Owner Author

travs commented Aug 19, 2017

@BlueHatbRit Wizard!
Thanks again Elliot.

You may see me coming for advice on testing for pdf output soon, haha.
I want to prevent the regression that happened to markdown-pdf before, since a fair number of people seem to use it.

For now I'll make the new release with mdpdf built in, and then (hopefully) smooth sailing! :)

@elliotblackburn
Copy link
Collaborator

@travs haha no worries!

Testing thats actually something I've struggled with a bit, validating pdf output is not easy. A lot of what I've done so far is just checking that the conversion process outputs a pdf. The problem is, it's quite hard to even pre-generate a pdf and diff the contents because (I believe) that pdf's contain some data with timestamps etc. I started reading through the pdf spec a few weeks back to see if I can figure out a way to check parts of a pdf properly. However the open spec is huge and I've not got far enough to come up with a plan yet.

It's definitely something worth thinking about for both our modules though.

Feel free to tag me in any issues that crop up if the release causes any issues for users, I'd be happy to take a look if they're getting really mental output that is todo with my module / the space in-between our projects.

@elliotblackburn
Copy link
Collaborator

Just scrolling through the issues on this repo, the addition of mdpdf should help resolve #147 really easily, worth tagging it here even though it's a bit older.

@elliotblackburn
Copy link
Collaborator

@travs I was just thinking, when this change is shipped I'll put a link to this module for atom users in the mdpdf readme. If that's alright with you, should I use the atom plugin page or the repo page? Your call, but I expect the atom plugin page is probably the best choice.

@travs
Copy link
Owner Author

travs commented Aug 29, 2017

@BlueHatbRit hey hey, just seeing this now.
Yes, the atom.io page would be better I think; thank you for doing this.
Have added your name and project to the top of the contributors in the README.md, but let me know if you want it somewhere else as well, and I'd be happy to oblige!

@elliotblackburn
Copy link
Collaborator

@travs oh that's great thanks, you did all the work so I wouldn't want my name anywhere else haha.

Here's the link back if you wanted to check it out -> elliotblackburn/mdpdf@8fade1b. I'll be pushing it to npm later on today.

@travs
Copy link
Owner Author

travs commented Aug 31, 2017

@BlueHatbRit Looks great, thanks!

@travs travs closed this as completed Sep 2, 2017
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

2 participants