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

New: Util chromium-finder #2264

Merged
merged 1 commit into from Apr 18, 2019
Merged

Conversation

molant
Copy link
Member

@molant molant commented Apr 16, 2019

Pull request checklist

Make sure you:

For non-trivial changes, please make sure you also:

  • Added/Updated related documentation.
  • Added/Updated related tests.

Short description of the change(s)

Fix #2260

I'll probably have to do more changes once I start working on #2139 and #2248 but this should be a good starting point.

@molant
Copy link
Member Author

molant commented Apr 16, 2019

I've changed the implementation of is-file based on the recommendations of the docs and that's probably why tests are failing. Probably a different error message? I'll take a look.

packages/utils/src/chromium-finder.ts Outdated Show resolved Hide resolved
packages/utils/src/chromium-finder.ts Outdated Show resolved Hide resolved
packages/utils/src/chromium-finder.ts Show resolved Hide resolved
packages/utils/src/fs/is-file.ts Outdated Show resolved Hide resolved
packages/utils/tests/chromium-finder.ts Outdated Show resolved Hide resolved
Copy link
Member

@antross antross left a comment

Choose a reason for hiding this comment

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

Looks good! I failed to find any actual issues so I'll leave you with a few nits. 😛

packages/utils/src/chromium-finder.ts Outdated Show resolved Hide resolved
packages/utils/tests/chromium-finder.ts Outdated Show resolved Hide resolved
packages/utils/src/chromium-finder.ts Outdated Show resolved Hide resolved
packages/utils/src/chromium-finder.ts Outdated Show resolved Hide resolved
packages/utils/src/chromium-finder.ts Outdated Show resolved Hide resolved
@molant molant force-pushed the new/chromium-finder branch 2 times, most recently from a8d4c92 to 46b6482 Compare April 18, 2019 02:57
@molant molant requested review from antross and sarvaje and removed request for antross April 18, 2019 02:59
@molant
Copy link
Member Author

molant commented Apr 18, 2019

Changed applied. Ready for a last round. I'll start working on puppeteer-core in extension-browser from this now that things seem to be pretty stable.

@molant molant mentioned this pull request Apr 18, 2019
3 tasks
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.

New util to find chromium browser [1]
3 participants