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: drop dependency on playwright-core in all our packages #2318

Merged
merged 2 commits into from
May 21, 2020

Conversation

aslushnikov
Copy link
Collaborator

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 #2268

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"
Copy link
Member

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?

# 4. Build root package unless --no-build was provided.

if [[ -z "${NO_BUILD}" ]]; then
npm run build
Copy link
Collaborator Author

@aslushnikov aslushnikov May 20, 2020

Choose a reason for hiding this comment

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

  1. types are missing
  2. .npmignore is missing
  3. do not npm run clean and do not npm run build
  4. inherit most fields in package.json

Also, rewrite in js to get a feeling

@aslushnikov aslushnikov force-pushed the remake-package-publishing branch from f1baa0c to f087031 Compare May 21, 2020 00:31
.npmignore Outdated
@@ -12,6 +12,10 @@ lib/injected/

# root for "playwright-core" package
Copy link
Contributor

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.

Copy link
Collaborator Author

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
Copy link
Contributor

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 :)

Copy link
Collaborator Author

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 :)

// 3. figure package description and browsers based on name
let description = '';
let whitelistedBrowsers = [];
if (packageName === 'playwright') {
Copy link
Contributor

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.

const toPath = path.join(packagePath, fileName);
cleanupPaths.push(toPath);
console.error(`- generating: //${path.relative(ROOT_PATH, toPath)}`);
await new Promise((resolve, reject) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use promisify?

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) => {
Copy link
Contributor

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)"
Copy link
Contributor

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
@aslushnikov aslushnikov force-pushed the remake-package-publishing branch from b7f4118 to a18cb07 Compare May 21, 2020 19:12
@aslushnikov aslushnikov merged commit 505d94a into microsoft:master May 21, 2020
@aslushnikov aslushnikov deleted the remake-package-publishing branch May 21, 2020 20:18
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.

[BUG] Types don't work in WebStorm ("Unresolved function or method")
3 participants