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(bidi): implement setInputFiles #34757

Merged
merged 1 commit into from
Feb 12, 2025

Conversation

yury-s
Copy link
Member

@yury-s yury-s commented Feb 12, 2025

This PR implements setInputFiles with file paths. The one that takes in-memory FilePayloads is not supported as Bidi protocol doesn't have corresponding command.

@yury-s yury-s requested a review from whimboo February 12, 2025 19:46
Comment on lines +537 to +538
const fromContext = toBidiExecutionContext(handle._context);
const shared = await fromContext.rawCallFunction('x => x', { handle: handle._objectId });
Copy link
Member

Choose a reason for hiding this comment

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

This is confusing, let's introduce proper layer and naming so that we could follow what handle, _objectId, etc mean.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do in a follow-up.

Copy link
Contributor

Test results for "tests 1"

7 flaky ⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/browsercontext-clearcookies.spec.ts:146:3 › should remove cookies by name and domain @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/proxy.spec.ts:44:3 › should use proxy for second page @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-leaks.spec.ts:82:5 › click should not leak @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-leaks.spec.ts:107:5 › fill should not leak @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-leaks.spec.ts:161:5 › waitFor should not leak @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-set-input-files.spec.ts:205:3 › should upload multiple large files @webkit-ubuntu-22.04-node18

38547 passed, 793 skipped
✔️✔️✔️

Merge workflow run.

@yury-s yury-s merged commit c2c336b into microsoft:main Feb 12, 2025
29 of 31 checks passed
@yury-s yury-s deleted the bidi-set-input-files branch February 12, 2025 20:49
Copy link
Collaborator

@whimboo whimboo left a comment

Choose a reason for hiding this comment

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

This is great! Thanks a lot for implementing this API. It was one that causes a lot of test failures. Lets see how many additional tests are going to pass now.

@@ -530,11 +530,21 @@ export class BidiPage implements PageDelegate {
}

async setInputFiles(handle: dom.ElementHandle<HTMLInputElement>, files: types.FilePayload[]): Promise<void> {
throw new Error('Method not implemented.');
throw new Error('Setting FilePayloads is not supported in Bidi.');
Copy link
Collaborator

Choose a reason for hiding this comment

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

@yury-s, can you please explain when this is used? First I thought that you can fake files for upload by specifying a virtual file path and the file contents but it looks like (when checking some examples in the docs) it's ok to just set the file name. So what exactly would be required here from BiDi?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is an overload in Playwright API that allows to set file payload as a tuple of (name, buffer, mime-type). This is similar to setting File() objects to the input. We could implement the overload by storing to a temp file in bidi, but given that mime-type would be decided by the browser based on the file name it's not 1-to-1.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually we can implement this one in user land, similar to juggler #34785

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