mermaid-js / mermaid-cli

Command line tool for the Mermaid library
MIT License
2.35k stars 227 forks source link

headless Puppeteer warning message shows #544

Closed earlAchromatic closed 1 year ago

earlAchromatic commented 1 year ago

Describe the bug Puppeteer warning message is logged which will then be shown when people use my app.

To Reproduce Steps to reproduce the behavior:

  1. Run mermaid cli (as cli or node api also)

Expected behavior No error messages shown. They come from puppeteer so quiet option does nothing.

Screenshots

  Puppeteer old Headless deprecation warning:
    In the near feature `headless: true` will default to the new Headless mode
    for Chrome instead of the old Headless implementation. For more
    information, please see https://developer.chrome.com/articles/new-headless/.
    Consider opting in early by passing `headless: "new"` to `puppeteer.launch()`
    If you encounter any bugs, please report them to https://github.com/puppeteer/puppeteer/issues/new/choose.
aloisklink commented 1 year ago

As a temporary measure, you can make a ./my-puppeteer-config.json file with {"headless": "new"} in it, and pass it to the CLI using --puppeteerConfigFile ./my-puppeteer-config.json.

Unfortunately, there's a pretty significant performance hit (my tests sometimes show a 10x slowdown!!), especially for PDF output, see https://github.com/puppeteer/puppeteer/issues/10071.

It also seems a lot less stable compared to the default headless mode, in my experience ({"headless": "new"} seems to crash/freeze a lot more often).

As mermaid-cli is often used to make .pdf files, I'm not sure if it's worth changing the default puppeteer config for mermaid-cli, just to hide a warning. However, if your use-case doesn't export PDFs, or you don't mind the performance hit, you're welcome to switch over to using {"headless": "new"}!

You could also pin your app to only use puppeteer versions before v19.11.0, e.g. something like npm install puppeteer@19.10.0.

jt-nti commented 1 year ago

Using --puppeteerConfigFile with a file containing {"headless": "old"} also gets rid of the warning.

I agree changing the default puppeteer config to use the new headless mode isn't worth the potential problems just to hide a warning, but I think explicitly setting the headless mode to old might be. Would that be possible?

aloisklink commented 1 year ago

I've found another work around, you can set the PUPPETEER_DISABLE_HEADLESS_WARNING environment variable when running mermaid-cli.

Using --puppeteerConfigFile with a file containing {"headless": "old"} also gets rid of the warning.

I agree changing the default puppeteer config to use the new headless mode isn't worth the potential problems just to hide a warning, but I think explicitly setting the headless mode to old might be. Would that be possible?

Hmmm, it's a bit of a hack, since it doesn't comply with Puppeteer's TypeScript types, which only accepts boolean | "new" | undefined. However, it does work with Puppeteer v19.11.2.

I had a look at the Puppeteer source-code, and it looks like "old" isn't a special value. Instead, any value that is truthy, but is non-true (and not "new") will get rid of the warning.

I've made a PR (see https://github.com/mermaid-js/mermaid-cli/pull/555). @jt-nti, since I adapted your idea, would you like to have a Co-authored-by credit, by me adding a Co-authored-by: James Taylor <3535067+jt-nti@users.noreply.github.com> git trailer to my commit's description?

jt-nti commented 1 year ago

@aloisklink, always happy to be associated with a bit of a hack, thank you :)

aloisklink commented 1 year ago

@aloisklink, always happy to be associated with a bit of a hack, thank you :)

Sorry, looks like that PR was merged before I had the chance to add you as a co-author. That's my fault, I should have left the PR as a draft until I got your reply :(

Well, on the plus side, at least you're now no longer associated with the // @ts-expect-error hack that I had to add to make TypeScript stop throwing error messages :)

jt-nti commented 1 year ago

I would definitely never do anything as hacky as // @ts-expect-error! :)

Thanks for getting a fix in so quickly.