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

Don't encourage binary string use #496

Closed
jimmywarting opened this issue Nov 10, 2016 · 2 comments
Closed

Don't encourage binary string use #496

jimmywarting opened this issue Nov 10, 2016 · 2 comments

Comments

@jimmywarting
Copy link
Contributor

jimmywarting commented Nov 10, 2016

as stated: https://developer.mozilla.org/en-US/docs/Web/API/FileReader/readAsBinaryString

This feature is non-standard and is not on a standards track. Do not use it on production sites facing the Web: it will not work for every user. There may also be large incompatibilities between implementations and the behavior may change in the future.

This method has been removed from the FileAPI standard. FileReader.readAsArrayBuffer() should be used instead.

It's not even supported in IE10

Javascript is bad at handling binary strings. One character is a 16-bit so it means a binary string can take up to 2x more memory since it's not in utf-8

What you should do in the readme is changing the examples to use arrayBuffer/typed arrays or just accept plain blob objects

@reviewher
Copy link
Contributor

@jimmywarting @miclill there was probably a reason for preferring readAsBinaryString, most likely performance. The demo supports both modes.

One character is a 16-bit so it means a binary string can take up to 2x more memory since it's not in utf-8

The binary string is a direct mapping of bytes (if s is the string, s.charCodeAt(i) is the i-th byte), so UTF8 conversion is not involved. The string length will be the exact byte size of the uploaded file.

What you should do in the readme is changing the examples to use arrayBuffer/typed arrays or just accept plain blob objects

I agree that it would be helpful to add more examples like the readAsArrayBuffer form.

@SheetJSDev
Copy link
Contributor

I have a 2010 tutorial that used readAsBinaryString in my bookmarks. When we added readAsArrayBuffer support, rABS was noticeably faster.

saarCiklum pushed a commit to Folcon/js-xlsx that referenced this issue Aug 17, 2020
- README and example cleanup
- basic XLSB and ODS write support
- flow typecheck for ODS file
  Note: xlsx.js flow fails: facebook/flow#380
- exposed jszip compression (fixes SheetJS#220, closes SheetJS#284)

README issues:

|  id  | author         | comment                                      |
|-----:|:---------------|:---------------------------------------------|
| SheetJS#202 | @sao93859      | closes SheetJS#202                                  |
| SheetJS#211 | @alexanderchan | closes SheetJS#211 corrected examples               |
| SheetJS#327 | @cskaandorp    | changed saveAs example to match write tests  |
| SheetJS#424 | @dskrvk        | added note about s2roa h/t @LeonardoPatignio |
| SheetJS#496 | @jimmywarting  | closes SheetJS#496 adapted rABS examples with rAAS  |

ODS file format issues:

|  id  | author         | comment                                      |
|-----:|:---------------|:---------------------------------------------|
| protobi#148 | @user4815162342| closes protobi#148 h/t @ziacik                      |
| protobi#166 | @paulproteus   | closes protobi#166 rudimentary ODS write support    |
| protobi#177 | @ziacik        | closes protobi#177                                  |
| protobi#179 | @ziacik        | closes protobi#179 use JSON when available          |
| SheetJS#317 | @ziacik        | closes SheetJS#317                                  |
| SheetJS#328 | @think01       | closes SheetJS#328                                  |
| SheetJS#383 | @mdamt         | closes SheetJS#383 duplicate cells should be copied |
| SheetJS#430 | @RB-Lab        | closes SheetJS#430                                  |
| SheetJS#546 | @lgodard       | closes SheetJS#546 thanks to other changes          |
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

No branches or pull requests

3 participants