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

fix: make more files work in strict-mode TypeScript #7936

Merged
merged 2 commits into from Jan 28, 2022
Merged

Conversation

OrKoN
Copy link
Collaborator

@OrKoN OrKoN commented Jan 28, 2022

Issues: #6769

@OrKoN OrKoN enabled auto-merge (squash) January 28, 2022 08:31
auto-merge was automatically disabled January 28, 2022 08:39

Pull Request is not mergeable

@OrKoN OrKoN enabled auto-merge (squash) January 28, 2022 08:55
@OrKoN OrKoN disabled auto-merge January 28, 2022 08:56
@OrKoN OrKoN enabled auto-merge (squash) January 28, 2022 08:56
@OrKoN OrKoN merged commit 0636513 into main Jan 28, 2022
@OrKoN OrKoN deleted the fix-ts-strict-mode branch January 28, 2022 09:38
@karlhorky
Copy link
Contributor

karlhorky commented Feb 4, 2022

Interesting, just merged this in the minor 13.1.3 update, and it broke our (previously working) CI with the error:

Error: puppeteerRootDirectory is not found.
    at initializePuppeteerNode
    at ../../node_modules/puppeteer/lib/cjs/puppeteer/node.js
    at __require2
    at ../../node_modules/puppeteer/cjs-entry.js
    at __require2

This is how we are launching Puppeteer:

await puppeteer.launch({
  executablePath: '/usr/bin/google-chrome-stable',  // Path to the Chrome binary on Linux
})

Should it break this way @OrKoN @mathiasbynens ?

@OrKoN
Copy link
Collaborator Author

OrKoN commented Feb 4, 2022

@karlhorky nope, that's not expected. We try to update Puppeteer to use strict-mode TypeScript and it looks like the types are lying to us here. Let me revert that part.

@OrKoN
Copy link
Collaborator Author

OrKoN commented Feb 4, 2022

actually, I think the types might be correct. I am not able to reproduce locally though but I suspect it is an issue similar to #7932? @karlhorky, could you share a reproduction?

@karlhorky
Copy link
Contributor

karlhorky commented Feb 4, 2022

My colleague @Josehower is working on this now, and will share a reproduction.

The basic idea behind the repro is: running a bundled Puppeteer (esbuild) using the code above on GitHub Actions (no npm install - just using the Chrome that is already installed on ubuntu-latest)

@Josehower
Copy link

Josehower commented Feb 4, 2022

HI, @OrKoN I am a colleague of @karlhorky thanks for the quick response and the attention to this issue. puppeteer is quite a nice tool that we use every day on our coding Bootcamp.

I have created this basic repo with a script that contains puppeteer and a GitHub workflow setup on issue comment.

you can find the error documented here.
https://github.com/Josehower/puppeteer-error-demo/runs/5070266538?check_suite_focus=true

file:///home/runner/work/puppeteer-error-demo/puppeteer-error-demo/upleveled-drone.mjs:26365
        throw new Error("puppeteerRootDirectory is not found.");
              ^

Error: puppeteerRootDirectory is not found.
    at initializePuppeteerNode (file:///home/runner/work/puppeteer-error-demo/puppeteer-error-demo/upleveled-drone.mjs:26365:15)
    at ../projects/node_modules/puppeteer/lib/cjs/puppeteer/node.js (file:///home/runner/work/puppeteer-error-demo/puppeteer-error-demo/upleveled-drone.mjs:26388:72)
    at __require2 (file:///home/runner/work/puppeteer-error-demo/puppeteer-error-demo/upleveled-drone.mjs:46:50)
    at ../projects/node_modules/puppeteer/cjs-entry.js (file:///home/runner/work/puppeteer-error-demo/puppeteer-error-demo/upleveled-drone.mjs:263[9](https://github.com/Josehower/puppeteer-error-demo/runs/5070266538?check_suite_focus=true#step:4:9)5:27)
    at __require2 (file:///home/runner/work/puppeteer-error-demo/puppeteer-error-demo/upleveled-drone.mjs:46:50)
    at file:///home/runner/work/puppeteer-error-demo/puppeteer-error-demo/upleveled-drone.mjs:26401:32
    at ModuleJob.run (node:internal/modules/esm/module_job:[18](https://github.com/Josehower/puppeteer-error-demo/runs/5070266538?check_suite_focus=true#step:4:18)5:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:281:24)
    at async loadESM (node:internal/process/esm_loader:88:5)
Error: Process completed with exit code 1.

if you want to test a bit the reproduction steps are:

  1. create a comment on the issue
  2. Check the log of the GitHub action triggered by the comment on the actions tab of the repo

you can even check the code for the script containing puppeteer v13.1.3

As you can see on the image the error is being ended from line 26364.

image

As additional information, I tried patching puppeteer with the possible solution presented on the PR 7933 whit no success.

we use a puppeteer bundled with esbuild and the base file with the script is a ts file.

again thanks for your time and looking forward to your answer.

Good Energy 👍👍👍👍

@OrKoN
Copy link
Collaborator Author

OrKoN commented Feb 7, 2022

@Josehower Thanks for the details. Could you try out the following CL #7967 ?

@Josehower
Copy link

@OrKoN Thanks ill try it and let you know

@Josehower
Copy link

@OrKoN I can confirm fix #7967 works fine, again thanks for the fast response.

looking forward to this PR being merged.

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

Successfully merging this pull request may close these issues.

None yet

4 participants