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

Fix: writeBinaryFile to call the correct command (fix #1133) #1136

Conversation

zakpatterson
Copy link
Contributor

@zakpatterson zakpatterson commented Jan 5, 2021

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • New Binding Issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes. Issue #___
  • No

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:
Fixes #1133.

Fix writeBinaryFile in the js api to call the correct function on the rust side. Before, files were sent in base64 encoding, but the save-as-text WriteFile fs command was reused, without any indication that the contents were base64 encoded. So the WriteFile command had no way of checking and decoding when necessary. WriteBinaryFile on the rust side expects a base64 encoded string, then decodes it.

@zakpatterson zakpatterson requested a review from a team as a code owner January 5, 2021 22:37
@zakpatterson zakpatterson requested a review from a team January 5, 2021 22:37
Copy link
Sponsor Member

@nothingismagick nothingismagick left a comment

Choose a reason for hiding this comment

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

Just please shorten the changefile and then will merge and kickoff a release. Thanks for tracking this down!

.changes/writeBinaryFile.md Outdated Show resolved Hide resolved
cli/tauri.js/api-src/fs.ts Show resolved Hide resolved
@nothingismagick
Copy link
Sponsor Member

I have reviewed this code and accept that the commits weren't signed. In the future, @zakpatterson, please consider signing commits with gpg.

https://docs.github.com/articles/about-gpg/

@zakpatterson
Copy link
Contributor Author

I have reviewed this code and accept that the commits weren't signed. In the future, @zakpatterson, please consider signing commits with gpg.

https://docs.github.com/articles/about-gpg/

Also happy to set that up and force push new commits if you want to give me a few minutes.

@nothingismagick
Copy link
Sponsor Member

Sure, will wait for you to force-push. Thanks @zakpatterson ❤️

Fixes tauri-apps#1133

writeBinaryFile was reusing writeFile, which was happily saving
base64 encoded strings to the fs. This instead uses the correct
WriteBinaryFile command, which base64 decodes.

However why are we encoding and then decoding, why can we not just
send a raw byte array to be saved as a file? This is left for a later
PR.
@zakpatterson zakpatterson force-pushed the fix/1133-tauri-js-api-writeBinaryFile branch from a2b140f to 13a15fe Compare January 6, 2021 08:12
@zakpatterson
Copy link
Contributor Author

Sure, will wait for you to force-push. Thanks @zakpatterson heart

Better?

@nothingismagick
Copy link
Sponsor Member

image
YES!

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.

writeBinaryFile **DOES NOT WORK** even though correct USAGE! [2021 ISSUE]
2 participants