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

Support locally installed puppeteer #19

Open
KylePDavis opened this issue Nov 8, 2017 · 5 comments
Open

Support locally installed puppeteer #19

KylePDavis opened this issue Nov 8, 2017 · 5 comments

Comments

@KylePDavis
Copy link

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?

@shd101wyy
Copy link
Owner

shd101wyy commented Nov 8, 2017

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?

@KylePDavis
Copy link
Author

KylePDavis commented Nov 8, 2017

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());

@shd101wyy
Copy link
Owner

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

@KylePDavis
Copy link
Author

KylePDavis commented Nov 26, 2017

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?

@shd101wyy
Copy link
Owner

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

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