-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Changed write
functions of FileSystemSyncAccessHandle to accept immutable buffers
#3537
Conversation
Removed unnecessary `mut` for the write functions.
write
functions of FileSystemSyncAccessHandle to accept immutable buffers
You might want to look at this section of the The part that is of interest is here: wasm-bindgen/crates/webidl/src/util.rs Lines 511 to 534 in b76af3b
By default slice references that get passed to JS are mutable. In order to make them immutable you need to whitelist them and then regenerate the API. |
LGTM! I'm not in the contributors list so I can't merge your PR, but maybe @daxpedda can. |
I noted that when I added this fix, it also modified the methods for IDBFileHandle, but when I go to MDN, the documentation for that class doesn't seem to exist anymore. Since it's not even available anyway, I don' think this should be an issue. My belief is that it should probably be removed if it's not even listed in MDN. |
This is a breaking change (playground) |
It might be a breaking change, but it's in the unstable API (which from what I can understand, when an API is unstable, we should expect breaking changes if necessary). In my specific use case, I am working with files that have sizes larger than I can load directly on the available RAM in a browser. In order to work on those files, I'm directly loading and saving into files after processing them in chunks. However, the chunks can still be quite large, and I already encountered files which would require chunks that, if I needed to allocate an intermediary buffer each time to change the mutability, would overflow the available memory. I believe I could import a function that would use the right signature myself, but the cleaner solution would be to fix the API. |
This isn't the case though, from client side the reference is already required to be fn original(b: &mut [u8]);
fn proposed(b: &[u8]); Calling |
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.
Apologies for my absence.
I believe this is technically still a breaking change because it can mess up inference, but I consider this a very minor breaking change.
It is also true that this is an experimental API where a breaking change is not an issue, but unfortunately the whitelisting also affected IdbFileHandle.write
, which isn't an experimental API, which is funny because this API is deprecated anyway.
All in all I'm happy to merge this.
Small nit: please add an entry to the changelog.
@hamza1311 I would appreciate your approval as well.
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.
Thank you!
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 had missed that it's part of unstable API as part of my previous review, sorry about that. This looks good to me.
Removed unnecessary
mut
for the write functions.According to the MDN documentation, there should not be any modification applied to the buffer, so this should be completely fine.
Should resolve #3536