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: improve compatibility of using ESM/TS with Deno #33128

Closed
wants to merge 3 commits into from

Conversation

nathanwhit
Copy link

@nathanwhit nathanwhit commented Oct 16, 2024

Deno does not support custom ESM loaders, which playwright uses to provide TS/ESM support. This currently means that deno users cannot use playwright with ESM. However, deno supports running typescript and ESM natively, so disabling the loader allows this to work.

A second small issue is that deno doesn't require a package.json file, and defaults to ESM (as opposed to node, which defaults to CommonJS). This means that currently if you don't have a package.json file, when running in deno playwright would incorrectly assume the project is commonjs.

Two small changes here:

  • Disable the ESM loader when running in deno (as detected by looking for process.versions.deno)
  • Consider files to be ESM, even without a package.json present, when running in deno

Overall this greatly improves the experience when using playwright with deno, and I hope the changes are minimal enough to be acceptable. Thanks for your time!

@nathanwhit
Copy link
Author

@microsoft-github-policy-service agree company="Deno Land Inc"

Copy link
Contributor

Test results for "tests 1"

5 flaky ⚠️ [chromium-library] › library/chromium/oopif.spec.ts:283:3 › should click @chromium-ubuntu-22.04-node18
⚠️ [playwright-test] › ui-mode-test-ct.spec.ts:93:5 › should run component tests after editing component @macos-latest-node18-1
⚠️ [webkit-library] › library/browsercontext-viewport-mobile.spec.ts:116:5 › mobile viewport › default mobile viewports to 980 width @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › library/screenshot.spec.ts:94:14 › page screenshot › should work with device scale factor and scale:css @webkit-ubuntu-22.04-node18
⚠️ [playwright-test] › ui-mode-test-watch.spec.ts:145:5 › should watch all @windows-latest-node18-1

35957 passed, 624 skipped
✔️✔️✔️

Merge workflow run.

@dgozman
Copy link
Contributor

dgozman commented Oct 16, 2024

@nathanwhit Thank you for the PR! We expect custom ESM loaders to become stable Node.js API soon, so that's probably some feedback to Deno to implement them and stay compatible with Node.js ecosystem.

For now, we'd prefer not to introduce Deno-specific code, since we haven't seen much interest from the community. I'd recommend to file an issue first, so that we can measure the community interest in supporting Deno.

I'll close this PR for now, since there are no immediate plans to land it. However, we can reopen it later if the Deno support becomes a widely requested feature.

@dgozman dgozman closed this Oct 16, 2024
@nathanwhit
Copy link
Author

nathanwhit commented Oct 16, 2024

@dgozman I respect the decision, but I would push back a bit on there not being interest – this issue requesting deno support from a few years back had dozens of upvotes and people commenting in support of the request.

Would you be open to landing just the one commit? 458e6c8
It's a one line change, and makes a big difference in usability for our users.

@marvinhagemeister
Copy link

FWIW we keep getting a couple of requests each week in the Deno discord asking for playwright support. We'd love to use playwright ourselves as well to test projects here at Deno. Merging this PR would unblock Deno users on being able to use playwright.

@dgozman
Copy link
Contributor

dgozman commented Oct 17, 2024

Would you be open to landing just the one commit? 458e6c8

As a workaround, you can just pass PW_DISABLE_TS_ESM environment variable today.

FWIW we keep getting a couple of requests each week in the Deno discord asking for playwright support.

Could you please file an issue? Unfortunately, it is hard to track requests for Playwright somewhere in Deno discord 😄 However, an issue would be a clear indicator of the interest and a great place for discussion. I assume we are talking exclusively about the recent Deno 2.0 release, and I don't think we've seen it explicitly mentioned on our issue tracker just yet.

Merging this PR would unblock Deno users on being able to use playwright.

Have you tried running with PW_DISABLE_TS_ESM? Is there any feedback? Does everything work? We are hesitant at landing partial support, because it's unclear whether there are more blocks down the road. For example, disabling the custom ESM loader breaks dependencies detection for watch mode, tsx preprocessor, our tsconfig path resolution, etc.

It would be interesting to run Playwright's own tests with deno and see what's passing and what's not. That could help us make an informed decision.

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.

3 participants