-
Notifications
You must be signed in to change notification settings - Fork 432
fix(fs): convert async iterator syntax to manual read #2550
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
Conversation
plugins/fs/guest-js/index.ts
Outdated
try { | ||
while (true) { | ||
const { done, value } = await reader.read() | ||
if (done) break | ||
await file.write(value) | ||
} | ||
} finally { | ||
reader.releaseLock() | ||
} | ||
|
||
await file.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should propagate the errors and not ignore them
try { | |
while (true) { | |
const { done, value } = await reader.read() | |
if (done) break | |
await file.write(value) | |
} | |
} finally { | |
reader.releaseLock() | |
} | |
await file.close() | |
try { | |
while (true) { | |
const { done, value } = await reader.read() | |
if (done) break | |
await file.write(value) | |
} | |
} catch(e) { | |
reader.releaseLock() | |
await file.close() | |
throw e | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amrbashir - with a try / finally
block, errors will still propagate as expected, no? Also, I think we should release the lock and close the file handle, regardless of an error, so it seems like finally
is the right place for this. Let me know your thoughts.
function test() {
try {
throw new Error("Oops!");
} finally {
console.log("Finally block runs!");
}
}
test(); // Logs "Finally block runs!", then throws "Oops!"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this does work indeed, thank you
Package Changes Through 493d5acThere are 6 changes which include deep-link with patch, deep-link-js with patch, http with patch, http-js with patch, fs with patch, fs-js with patch Planned Package VersionsThe following package releases are the planned based on the context of changes in this pull request.
Add another change file through the GitHub UI by following this link. Read about change files or the docs at github.com/jbolda/covector |
Also could you add a change file in Also you need to run Thank you |
Thank you |
The
writeFile
function accepts aReadableStream
, however, under macOS this does not work since the built-in webview does not have async iterator support. See Safari's compatibility for more info.This PR converts the code to a manual read while-loop instead, fixing support for macOS.
Note: I have not tested this across all platforms. Perhaps @amrbashir could check this, if possible? 🙏