crossnote icon indicating copy to clipboard operation
crossnote copied to clipboard

Support locally installed puppeteer

Open KylePDavis opened this issue 7 years ago • 5 comments

The PDF generation via Chrome is great but I would like to package puppeteer locally instead of being required to have it installed globally (this can problematic in some scenarios).

It looks like the initial implementation used the requireg package and was then adjusted to with a simpler local implementation in PR #9 due to some macOS compatibility issues.

Maybe if we could get a little closer to the original requireg implementation we could allow for local puppeteer installations as well. If we change the chromeExport() method to try a native require('puppeteer') first and then use the current global check as a fallback then we could support both cases (see src/markdown-engine.ts#L1364-L1370).

Thoughts?

KylePDavis avatar Nov 08 '17 04:11 KylePDavis

Hello @KylePDavis ,

I think puppeteer is too big to be installed locally. The mume project is used in vscode-markdown-preview-enhanced project, which is an extension for visual studio code. However, visual studio code has file size limit for its extension. If I put puppeteer inside package.json and install it locally, then the file size of packed vscode extension will exceed. That's why I decide to make it require globally.

However, it's quite hard to require puppeteer globally because it's hard to find the path of where it is installed. Apparently requireg doesn't work well. I am using nvm, and it seems that requireg returns me the wrong path. And it doesn't work well on Windows either.

Therefore, my plan is to let user enter the path of puppeteer like:

const engine = new mume.MarkdownEngine({
    filePath: "/Users/wangyiyi/Desktop/markdown-example/test3.md",
    config: {
      previewTheme: "github-light.css",
      puppeteerPath: "/usr/local/node_modules/puppeteer"
    }
  })

What do you think?

shd101wyy avatar Nov 08 '17 05:11 shd101wyy

Thanks for the quick response! I definitely think that puppeteer is too big to be included with markdown-preview-enhanced. On the other hand, I don't think that mume should prevent it's consumers from doing that if they want to.

In my case, I'm trying to use mume as a dependency and want to also depend on puppeteer. Since what I'm doing doesn't have the same restrictions, it's perfectly reasonable to package it that way (and anyone depending on my package could tweak the puppeteer install via a few environment variables if needed).

Your idea for providing an option for where puppeteer might work, and might even be necessary for some cases, but I hope not. I'd really like to see if we can come up with something better before making consumers do extra work.

I'm thinking something as simple as trying to resolve against a list of paths. Then maybe if there are still edge cases you could allow them to override that list of module directories but hopefully that won't be necessary. What about trying the list of paths provided by the global-paths package?

Then you could do something like this:

const path = require('path');
const getGlobalPaths = require('global-paths');

function requireFromPaths(id, paths) {
  for (let p of paths) {
    try { return require(path.join(p, id)); } catch(err) { /* next... */ }
  }
  throw new Error(`Unable to load "${id}" from: ${paths.join(',')}`)
}

// ... some where later in the code ...

puppeteer = requireFromPaths('puppeteer', getGlobalPaths());

KylePDavis avatar Nov 08 '17 07:11 KylePDavis

Hi @KylePDavis ,

This issue should be fixed now. Please check this line.
Basically I call command npm root -g to get the path to global node_modules directory.
Then I require puppeteer from that.
So if you have puppeteer installed globally by npm install -g puppeteer, then the chrome (puppeteer) export should work.

Thanks

shd101wyy avatar Nov 26 '17 07:11 shd101wyy

Hey @shd101wyy, thanks for looking into this. It looks like that code forces global module resolution but here I actually need the ability to use the standard local module resolution.

It would be much better for me if I could simply use the version of puppeteer that I have specified in my package's package.json file. This is much more reliable than hoping that the user has already globally-installed a compatible version of the puppeteer module.

In other words, I want to prefer local modules first but then fallback to global modules.

I see two viable approaches here:

  1. allow users of the mume package to specify a puppeteerPath config option (the module path)
  2. modify the mume package to search for local modules and then global modules upon failure

Long-term I think approach [2] is better but short-term I could make approach [1] work if you would rather do that instead. Even longer-term it might even make sense to do both approach [2] and approach [1].

Does that better explain what I'm after here?

KylePDavis avatar Nov 26 '17 08:11 KylePDavis

It would be great if you can submit me a pull request.

shd101wyy avatar Nov 26 '17 08:11 shd101wyy