Skip to content

fix(fs): convert async iterator syntax to manual read - #2550

Merged
amrbashir merged 3 commits into
tauri-apps:v2from
tjramage:v2
Mar 21, 2025
Merged

fix(fs): convert async iterator syntax to manual read#2550
amrbashir merged 3 commits into
tauri-apps:v2from
tjramage:v2

Conversation

@tjramage

Copy link
Copy Markdown
Contributor

The writeFile function accepts a ReadableStream, 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? 🙏

@tjramage
tjramage requested a review from a team as a code owner March 19, 2025 19:08
Comment thread plugins/fs/guest-js/index.ts Outdated
Comment on lines 1080 to 1090
try {
while (true) {
const { done, value } = await reader.read()
if (done) break
await file.write(value)
}
} finally {
reader.releaseLock()
}

await file.close()

@amrbashir amrbashir Mar 19, 2025

Copy link
Copy Markdown
Member

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

Suggested change
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
}

Copy link
Copy Markdown
Contributor Author

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!"

Copy link
Copy Markdown
Member

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

@github-actions

github-actions Bot commented Mar 19, 2025

Copy link
Copy Markdown
Contributor

Package Changes Through 493d5ac

There 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 Versions

The following package releases are the planned based on the context of changes in this pull request.

package current next
api-example 2.0.23 2.0.24
api-example-js 2.0.19 2.0.20
deep-link-example-js 2.2.0 2.2.1
deep-link 2.2.0 2.2.1
deep-link-js 2.2.0 2.2.1
fs 2.2.0 2.2.1
fs-js 2.2.0 2.2.1
dialog 2.2.0 2.2.1
dialog-js 2.2.0 2.2.1
http 2.4.2 2.4.3
http-js 2.4.2 2.4.3
persisted-scope 2.2.0 2.2.1
single-instance 2.2.2 2.2.3

Add another change file through the GitHub UI by following this link.


Read about change files or the docs at github.com/jbolda/covector

@amrbashir

Copy link
Copy Markdown
Member

Also could you add a change file in .changes directory? see https://github.com/tauri-apps/plugins-workspace/blob/v2/.changes/persist-cookies.md?plain=1 for an example.

Also you need to run pnpm build and pnpm format

Thank you

@tjramage
tjramage requested a review from amrbashir March 20, 2025 13:19
@amrbashir
amrbashir merged commit 831c35f into tauri-apps:v2 Mar 21, 2025
@amrbashir

Copy link
Copy Markdown
Member

Thank you

gezihuzi pushed a commit to Hypobenthos/plugins-workspace that referenced this pull request Jun 22, 2025
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.

2 participants