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

chore: log useful error for Node v14 breakage #5732

Merged
merged 3 commits into from Apr 27, 2020
Merged

chore: log useful error for Node v14 breakage #5732

merged 3 commits into from Apr 27, 2020

Conversation

jackfranklin
Copy link
Collaborator

@jackfranklin jackfranklin commented Apr 24, 2020

There is currently a bug with extract-zip and Node v14.0.0 that
causes extractZip to silently fail:
#5719

Rather than silenty fail if the user is on Node 14 we instead
detect that and throw an error directing the user to that bug. The
rejection message below is surfaced to the user in the command line.

The issue seems to be in streams never resolving so we wrap the call in
a timeout and give it 100ms to resolve before deciding on an error. If
the user is on Node < 14 we maintain the behaviour we had before this
patch.

Here's how this change impacts the output on Node 14 and Node 10:

Node 10:

npm run tsc && rm -r .local-* && node install

> puppeteer@3.0.1-post tsc /Users/jacktfranklin/src/puppeteer
> tsc --version && tsc -p . && cp src/protocol.d.ts lib/ && cp src/externs.d.ts lib/

Version 3.8.3
Downloading Chromium r737027 - 118.4 Mb [====================] 100% 0.0s
Chromium (737027) downloaded to /Users/jacktfranklin/src/puppeteer/.local-chromium/mac-737027

Node 14 without this patch:

npm run tsc && rm -r .local-* && node install

> puppeteer@3.0.1-post tsc /Users/jacktfranklin/src/puppeteer
> tsc --version && tsc -p . && cp src/protocol.d.ts lib/ && cp src/externs.d.ts lib/

Version 3.8.3
Downloading Chromium r737027 - 118.4 Mb [====================] 100% 0.0s

Note that whilst it doesn't error, it doesn't complete the install. We
don't get the success message that we saw above in the Node 10 install.

Node 14 with this patch:


> puppeteer@3.0.1-post tsc /Users/jacktfranklin/src/puppeteer
> tsc --version && tsc -p . && cp src/protocol.d.ts lib/ && cp src/externs.d.ts lib/

Version 3.8.3
Downloading Chromium r737027 - 118.4 Mb [====================] 100% 0.0s
ERROR: Failed to set up Chromium r737027! Set "PUPPETEER_SKIP_DOWNLOAD" env variable to skip download.
Puppeteer currently does not work on Node v14 due to an upstream bug. Please see https://github.com/puppeteer/puppeteer/issues/5719 for details.

The explicit message should save users a good amount of debugging time.

There is currently a bug with extract-zip and Node v14.0.0 that
causes extractZip to silently fail:
#5719

Rather than silenty fail if the user is on Node 14 we instead
detect that and throw an error directing the user to that bug. The
rejection message below is surfaced to the user in the command line.

The issue seems to be in streams never resolving so we wrap the call in
a timeout and give it 100ms to resolve before deciding on an error. If
the user is on Node < 14 we maintain the behaviour we had before this
patch.

Here's how this change impacts the output on Node 14 and Node 10:

Node 10:

```
npm run tsc && rm -r .local-* && node install

> puppeteer@3.0.1-post tsc /Users/jacktfranklin/src/puppeteer
> tsc --version && tsc -p . && cp src/protocol.d.ts lib/ && cp src/externs.d.ts lib/

Version 3.8.3
Downloading Chromium r737027 - 118.4 Mb [====================] 100% 0.0s
Chromium (737027) downloaded to /Users/jacktfranklin/src/puppeteer/.local-chromium/mac-737027
```

---

Node 14 without this patch:

```
npm run tsc && rm -r .local-* && node install

> puppeteer@3.0.1-post tsc /Users/jacktfranklin/src/puppeteer
> tsc --version && tsc -p . && cp src/protocol.d.ts lib/ && cp src/externs.d.ts lib/

Version 3.8.3
Downloading Chromium r737027 - 118.4 Mb [====================] 100% 0.0s
```

Note that whilst it doesn't error, it doesn't complete the install. We
don't get the success message that we saw above in the Node 10 install.

---

Node 14 with this patch:

```npm run tsc && rm -r .local-* && node install

> puppeteer@3.0.1-post tsc /Users/jacktfranklin/src/puppeteer
> tsc --version && tsc -p . && cp src/protocol.d.ts lib/ && cp src/externs.d.ts lib/

Version 3.8.3
Downloading Chromium r737027 - 118.4 Mb [====================] 100% 0.0s
ERROR: Failed to set up Chromium r737027! Set "PUPPETEER_SKIP_DOWNLOAD" env variable to skip download.
Puppeteer currently does not work on Node v14 due to an upstream bug. Please see #5719 for details.
```

The explicit message should save users a good amount of debugging time.
* If the user is on Node < 14 we maintain the behaviour we had before
* this patch.
*/
if (nodeVersion === 'v14.0.0') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nodeVersion.startsWith('v14.')?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or do you think this is something Node.js will ship a patch for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hopeful that Node will ship a patch for it (given nodejs/node#32968).

But it could be safer to default to starts with v14. for now and then we can just ship a new release when the problem is solved, so let's do that 👍

src/BrowserFetcher.ts Outdated Show resolved Hide resolved
@munrocket
Copy link
Contributor

Yes this is much better, but IMHO puppeteer should work like an axe in any OS and nodejs.

Don’t know what’s happened after 2.2.1 but in future better to avoid such bugs.
For example in my repo every contributor should make a screenshot after PR, I don’t want help to everybody how to install packages with npm :D

@mathiasbynens mathiasbynens changed the title chore: Useful error for Node v14 breakage chore: add useful error for Node v14 breakage Apr 27, 2020
@mathiasbynens mathiasbynens changed the title chore: add useful error for Node v14 breakage chore: log useful error for Node v14 breakage Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants