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: migrate @web to @playwright/web #34013

Closed
wants to merge 1 commit into from

Conversation

mxschmitt
Copy link
Member

@mxschmitt mxschmitt commented Dec 13, 2024

This is the first iteration on an attempt to get rid of local tsconfig package aliases in order to simplify the imports. It uses the more common package name imports what NPM workspaces offer out of the box. This also helps us to adopt tsconfig package references (not decided yet) and makes the boundaries clearer between the packages.

The next iterations would be to do something similar for:

  • @html-reporter -> rename to @playwright/html-reporter
  • @protocol -> rename to @playwright/protocol
  • @recorder -> rename to @playwright/recorder
  • @trace -> rename to @playwright/trace

@mxschmitt mxschmitt force-pushed the migrate-web-to-package branch from 7615d87 to 84838d6 Compare December 13, 2024 23:12

This comment has been minimized.

@mxschmitt mxschmitt force-pushed the migrate-web-to-package branch from 84838d6 to fc1e986 Compare December 13, 2024 23:34

This comment has been minimized.

@mxschmitt mxschmitt marked this pull request as ready for review December 13, 2024 23:42
Copy link
Contributor

Test results for "tests 1"

4 flaky ⚠️ [firefox-page] › page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [webkit-library] › library/screenshot.spec.ts:96:14 › page screenshot › should work with device scale factor and scale:css @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › library/trace-viewer.spec.ts:169:1 › should complain about newer version of trace in old viewer @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › page/page-set-input-files.spec.ts:147:3 › should upload large file @webkit-ubuntu-22.04-node18

37292 passed, 650 skipped
✔️✔️✔️

Merge workflow run.

@@ -1,6 +1,6 @@
[*]
@playwright/experimental-ct-react
@web/**
@playwright/web/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@playwright/web/**
@playwright/web

@@ -19,7 +19,7 @@ import './chip.css';
import './colors.css';
import './common.css';
import * as icons from './icons';
import { clsx } from '@web/uiUtils';
import { clsx } from '@playwright/web/src/uiUtils';
Copy link
Contributor

Choose a reason for hiding this comment

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

In playwright we import stuff from playwright-core differently:

import { formatCallLog } from 'playwright-core/lib/utils';

Similarly, @playwright/experimental-ct-core imports from @playwright/test differently:

import { Watcher } from 'playwright/lib/fsWatcher';

Why is this one different?

if (packages.has(package))
importPath = packages.get(package) + tokens.slice(1).join('/');
if (packages.has(`@${tokens[0]}/${tokens[1]}`)) {
// Namespaced package.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "namespaced" mean here?

importPath = packages.get(package) + tokens.slice(1).join('/');
if (packages.has(`@${tokens[0]}/${tokens[1]}`)) {
// Namespaced package.
importPath = packages.get(`@${tokens[0]}/${tokens[1]}`) + tokens.slice(2).join('/');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this different from other packages, e.g. playwright-core? I'd leave importPath undefined and go the line 162 old code instead.

@@ -192,8 +202,9 @@ async function innerCheckDeps(root) {
}
if (line === '***')
group.push('***');
else if (line.startsWith('@'))
group.push(line.replace(/@([\w-]+)\/(.*)/, (_, arg1, arg2) => packages.get(arg1) + arg2));
else if (line.startsWith('@')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • I don't really get this change.
  • styling is off 😄

@@ -14,7 +14,7 @@
* limitations under the License.
*/

import { ansi2html } from '@web/ansi2html';
import { ansi2html } from '../ansi2html';
Copy link
Contributor

Choose a reason for hiding this comment

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

Great fixes in packages/web!

@mxschmitt mxschmitt closed this Dec 18, 2024
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.

2 participants