-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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: drop dependency on playwright-core in all our packages #2318
chore: drop dependency on playwright-core in all our packages #2318
Conversation
packages/build_package.sh
Outdated
CLEANUP_PATHS+=("${README_PATH}") | ||
cp ../README.md "${README_PATH}" | ||
elif [[ "${PACKAGE_NAME}" == "playwright-core" ]]; then | ||
PACKAGE_DESCRIPTION="A high-level API to automate web browsers" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we put a note that this package should not be used directly?
packages/build_package.sh
Outdated
# 4. Build root package unless --no-build was provided. | ||
|
||
if [[ -z "${NO_BUILD}" ]]; then | ||
npm run build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- types are missing
- .npmignore is missing
- do not
npm run clean
and do notnpm run build
- inherit most fields in
package.json
Also, rewrite in js to get a feeling
f1baa0c
to
f087031
Compare
.npmignore
Outdated
@@ -12,6 +12,10 @@ lib/injected/ | |||
|
|||
# root for "playwright-core" package |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we are here, let's update this comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -12,6 +12,10 @@ lib/injected/ | |||
|
|||
# root for "playwright-core" package | |||
!index.js | |||
!install.js | |||
!README.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should read the first line of this file and enjoy :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
: D whitelisting doesn't hurt, I guess :)
packages/build_package.js
Outdated
// 3. figure package description and browsers based on name | ||
let description = ''; | ||
let whitelistedBrowsers = []; | ||
if (packageName === 'playwright') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use an object with 'playwright-foo' keys.
packages/build_package.js
Outdated
const toPath = path.join(packagePath, fileName); | ||
cleanupPaths.push(toPath); | ||
console.error(`- generating: //${path.relative(ROOT_PATH, toPath)}`); | ||
await new Promise((resolve, reject) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use promisify?
packages/build_package.js
Outdated
const toPath = path.join(packagePath, fileOrDirectoryName); | ||
cleanupPaths.push(toPath); | ||
console.error(`- copying: //${path.relative(ROOT_PATH, fromPath)} -> //${path.relative(ROOT_PATH, toPath)}`); | ||
await new Promise((resolve, reject) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
npm publish packages/playwright-webkit --tag="${NPM_PUBLISH_TAG}" | ||
npm publish packages/playwright-chromium --tag="${NPM_PUBLISH_TAG}" | ||
npm publish packages/playwright --tag="${NPM_PUBLISH_TAG}" | ||
PLAYWRIGHT_TGZ="$(node ./packages/build_package.js playwright ./playwright.tgz)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to update the recipe on the bot to build beforehand?
This patch: - drops dependency on playwright-core in all our packages. Instead of the dependency, packages are now built with `//packages/build_package.sh` script. - unifies `browsers.json` - now there's a single `//browsers.json` file that is used to manage browser revisions. This patch temporary switches canary publishing to `--dryn-run` from CI/CD so that we can verify that it does sane things. We'll unify all our package management scripts under `//packages/` in a follow-up. Fixes microsoft#2268
b7f4118
to
a18cb07
Compare
This patch:
the dependency, packages are now built with
//packages/build_package.sh
script.
browsers.json
- now there's a single//browsers.json
filethat is used to manage browser revisions.
This patch temporary switches canary publishing to
--dryn-run
from CI/CD so that wecan verify that it does sane things.
We'll unify all our package management scripts under
//packages/
in afollow-up.
Fixes #2268