-
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
[BUG] Chrome downloads fail despite acceptDownloads
set
#1777
Comments
Looking at this further, what I though might be a bug might actually just be a question since it actually looks like I'm using Could you clarify (either here) or add an example in the documentation of getting the download path from within the If I run the following example which more closely follows the docs:
The download path is printed and no error is trigger. What's the difference between the download object in If I change the original code to:
The path does get printed; however, this seems like a bad practice since it waits for arbitrary instead of an event (but if I wait for Thanks! |
In your original snippet, You should make sure control flow waits for the download to happen. The Please feel free to reopen if something was missing in my response! |
Thanks for your quick response! Could you clarify how you can wait for download to actually finish within the While the
Is it possible to wait for the download to actually finish from with the In my original snippet:
I assumed the I'm assuming the following are equivalent:
|
I am facing the same issue, I can't wrap my head around why playwright asks for my snippet is: return new Promise((accept,reject) => {
this.page.once('download', download => {
download.path().then((file) => {
let rawJSON = readFileSync(file,{encoding: 'utf8'})
accept(JSON.parse(rawJSON))
}).catch(e => reject(e))
})
//click on a button that triggers the download
}) I tried both non async and async context. This behavior is not limited to chromium, firefox is also not able to download the file, but it does not fire the "download" event at all, it instead displays the message " could not be saved, because the source file could not be read.". It might be helping to know that I'm trying to download a |
Can you paste complete snippet including the code where you create the context and the page?
A code snippet would be helpful here as well. Can it be that the blob gets garbage collected by the browser before you start downloading it? |
I am going to fiddle with this possible bug tomorrow and open a new issue if I find out it is a bug related to |
@pavelfeldman / @yury-s: It doesn't look like I have permission to re-open this issue, but I think I figured out what is happening (at least for the originally reported issue). It looks to be that an I've included a patch at the end. If you don't see any red flags, let me know and I will contribute an MR (including some additional tests). Here's the current [ constructor(page: Page, downloadsPath: string, uuid: string, url: string) {
this._page = page;
this._downloadsPath = downloadsPath;
this._uuid = uuid;
this._url = url;
this._finishedCallback = () => {};
this._finishedPromise = new Promise(f => this._finishedCallback = f);
for (const barrier of this._page._frameManager._signalBarriers)
barrier.addDownload();
this._page.emit(Events.Page.Download, this);
page._browserContext._downloads.add(this);
this._acceptDownloads = !!this._page._browserContext._options.acceptDownloads;
} Notice how the event is emitted this._page.emit(Events.Page.Download, this); before
. If I move the event emission to the end of the constructor, RAW Debugging Notes Start with The following doesn't work and you'll get a misleading error message ( (async () => {
for (const browserType of ['chromium']) {
const browser = await playwright[browserType].launch();
// [RAW] NB: Ensure acceptDownloads is set per https://github.com/microsoft/playwright/blob/bb0b6cd90a9f95c2337ddfcafb516c48318e4c43/docs/api.md#class-download
const context = await browser.newContext({acceptDownloads: true });
const page = await context.newPage();
page.on("download", async (dl) => {
const path = await dl.path();
console.log(path);
});
await page.goto("https://upbeat-nightingale-6e32da.netlify.com/download");
await page.click("text=Download");
// Do some other processing…
await page.waitFor(10_000);
await browser.close();
}
})(); But if you add an "internal" guard: diff --git a/index.js b/index2.js
index 75b3e8a..f48d450 100644
--- a/index.js
+++ b/index2.js
@@ -9,6 +9,7 @@ const playwright = require('playwright');
page.on("download", async (dl) => {
+ await new Promise(res => setTimeout(res, 200));
const path = await dl.path();
console.log(path);
}); everything works and you'll get the path printed. Now, that's not very robust, let's wait on the internal download promise:
This also works. This then lead into me diving into the Download implementation and discovering the "race" between the event emission vs. acceptDownloads being set. If we patch diff --git a/src/download.ts b/src/download.ts
index 200bd75..26047a4 100644
--- a/src/download.ts
+++ b/src/download.ts
@@ -41,9 +41,9 @@ export class Download {
this._finishedPromise = new Promise(f => this._finishedCallback = f);
for (const barrier of this._page._frameManager._signalBarriers)
barrier.addDownload();
- this._page.emit(Events.Page.Download, this);
page._browserContext._downloads.add(this);
this._acceptDownloads = !!this._page._browserContext._options.acceptDownloads;
+ this._page.emit(Events.Page.Download, this);
}
url(): string { We don't need the random sleep or the waiting on |
@SeekDaSky I just confirmed that what I said in #1777 (comment) also applies to the following <!DOCTYPE html>
<html>
<head>
<title>SeekDaSky Blob Example</title>
</head>
<body>
<script>
const download = (data, filename) => {
const a = document.createElement("a");
a.style = "display: none";
document.body.appendChild(a);
a.style = "display: none";
const json = JSON.stringify(data);
const blob = new Blob([json], { type: "octet/stream" });
const url = window.URL.createObjectURL(blob);
a.href = url;
a.download = filename;
a.click();
window.URL.revokeObjectURL(url);
document.body.removeChild(a);
};
const downloadIt = () => {
download({ foo: 1, bar: 2}, "example.json");
}
</script>
<a onclick="javascipt:downloadIt();">Download</a>
</body>
</html> In each of the examples, you can sub the target url for https://c7ecc89a4366bf00.netlify.app/seekdasky-example (which hosts the above file) and the same conclusions apply. |
This patch fixes `download.path()` throwing an error that `acceptDownloads` is not set when using a download within a `page.on` handler even when `acceptDownloads` had been set to `true`; Fixes microsoft#1777
This patch fixes `download.path()` throwing an error that `acceptDownloads` is not set when using a download within a `page.on` handler even when `acceptDownloads` had been set to `true`; Fixes microsoft#1777
Context:
node v13.12.0
Code Snippet
Output:
Describe the bug
Despite passing
{ acceptDownloads: true }
to the browser context (per the docs and error message), downloading fails complaining thatacceptDownloads
is not set.Expected Behavior:
Archive.zip
is downloaded.Misc.
Thanks for this amazing project! It's awesome to see the rapid feature development and progress! 🎉
The text was updated successfully, but these errors were encountered: