-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Add Bun support #28875
Add Bun support #28875
Conversation
This adds a `package.json` `"exports"` condition for Bun which always chooses the CommonJS version of the export (for both ESM & CJS). This package uses `__esModule` to tell bundlers that it was ESM converted into CommonJS. Bun supports this both at runtime and when using the bundler, but Node.js does not support this at runtime and that causes a compatibility issue when importing `@playwright/test` (which is our fault). It's unfortunately a compatibility issue whether or not we support `__esModule`, because choosing to not support it breaks libraries which presently only work when using a bundler. Since Bun automatically converts CommonJS <> ESM and `module.exports` with dynamic properties are supported, we can choose to always use the CommonJS entry point for Playwright in Bun. There were other changes necessary to get Playwright to work, which are mostly in oven-sh/bun#7958 Signed-off-by: Jarred Sumner <[email protected]>
Test results for "tests 1"2 flaky14894 passed, 307 skipped Merge workflow run. |
Trying to understand the core of the issue. Wouldn't this be a problem for any compiled library that exports both CJS and ESM variants? I.e. shouldn't this be addressed on the Bun side for Node interoperability? |
Related to #7328. Instead of tweaking the configs. What if we just remove: playwright/packages/playwright/test.js Line 24 in 4b99d21
It was added as part of #7328 to work with https://www.typescriptlang.org/tsconfig#esModuleInterop but it may not be needed (could be an extra bit thrown in) |
UpdateI tested the one line removal against the lastest playwright tag v1.40.1 and of the 9,874 browser tests and 1,291 playwright tests only a baker's dozen 🍞 of flapper test fails appeared so removing
from packages/playwright/test.js#L24 looks like an option 🙌 🤠 From the playwright init example: And with multiple workers: |
@jdalton that's a nice finding. This line predates our ESM support and I don't see why we would need it today. I'll run all the bots w/o this line. This breakage still suggests that there is an interoperability problem for Bun vs Node, not sure how important it is to the Bun folks though. |
Awesome! 🕺
This breakage is related to Object.defineProperty(combinedExports, '__esModule', { value: true }); The issue is by design though the heuristic when to respect the |
One test failed for us without it: import t from '@playwright/test';
t('passed', () => {
t.expect(1 + 1).toBe(2);
}); We compile Playwright using Babel. We also transpile TS tests on the fly using Babel. The Babel-to-babel interop works for us out of the box via __esModule: // https://babeljs.io/docs/babel-plugin-transform-modules-commonjs#strict
Object.defineProperty(combinedExports, '__esModule', { value: true }); and function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; } on the importing side. In case of Bun, it takes over the transpilation of TS, so it seems like we need to align on the convention we are using. If Bun acts as Babel and trusts __esModule to already have Side note: |
I landed #28935, could you confirm it addresses your scenario? |
This should work once microsoft/create-playwright#108 has been landed and released: It was otherwise using npx/npm internally. bun create playwright --next buntest
cd buntest
bun --bun x playwright install --with-deps
bun --bun x playwright test Sometimes getting |
🎉 🎉 🎉
I've got it on my roadmap to make Bun work with
✅ Fixed in https://bun.sh/blog/bun-v1.0.22 |
I suspect the worker issue still happens but much less frequently than before. Our |
Closing this PR then. We can have a new tracking meta-issue for Bun in the Issues to discuss problems like this one! |
Is there already a new meta-issue? Or should the old Bun issue be reopened? |
With Bun support, I'm wondering whether that would also unlock custom loaders in Playwright, as community members such as @AlessioGr, @leotg130, @divmgl and @madiodio have asked for in a few places: |
Fixes #27139
This adds a
package.json
"exports"
condition for Bun which always chooses the CommonJS version of the module (for both ESM & CJS).The built version of playwright uses
__esModule
to tell bundlers the code was ESM transplied into CommonJS. Bun supports this both at runtime and when using the bundler, but Node.js does not support this at runtime and that causes a compatibility issue when importing@playwright/test
. It's unfortunately a compatibility issue whether or not we support__esModule
, because choosing to not support it breaks libraries which otherwise only work when using a bundler.Since Bun automatically converts CommonJS <> ESM and
module.exports
with dynamic properties are supported, we can choose to always use the CommonJS entry point for Playwright in Bun.There were other changes necessary to get Playwright to work, which are mostly in oven-sh/bun#7958. After this PR, the remaining blocker for using Playwright in Bun is a stability issue when multiple workers are in use, which we are working on fixing.