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

Why fatal option is not supported? #24

Open
khoanguyen-yang opened this issue Nov 3, 2022 · 9 comments · May be fixed by #29
Open

Why fatal option is not supported? #24

khoanguyen-yang opened this issue Nov 3, 2022 · 9 comments · May be fixed by #29

Comments

@khoanguyen-yang
Copy link

Hi,

I am wondering why the fatal option is not supported. Will setting fatal=true will cause any issue when using your library as a polyfill?

Many thanks

@samthor
Copy link
Owner

samthor commented Nov 3, 2022

Setting fatal=true will cause the polyfill to fail since it's not supported.

I could probably add support, it wouldn't be too hard.

@khoanguyen-yang
Copy link
Author

khoanguyen-yang commented Nov 4, 2022

But it seems the fatal option is not used at all in the library

fatal is always set to false here https://github.com/samthor/fast-text-encoding/blob/master/src/o-decoder.js#L51
But the validation above it rejects the fatal option. https://github.com/samthor/fast-text-encoding/blob/master/src/o-decoder.js#L35
So wondering why you need the validation when you do not use it at all?


FYI, I am currently use your library to polyfill encoder/decoder in React Native and some other libraries do use the fatal option. I made a simple patch to disable only the validation (leaving this.fatal=false unchanged) and it seems to be working fine.
However I am not sure your library will work 100% ok for that change. So it would be nice if I have your confirmation or if you have time, you can make a change to the library if possible and necessary.

@pablodenadai
Copy link

Setting fatal=true will cause the polyfill to fail since it's not supported.
I could probably add support, it wouldn't be too hard.

That'd be awesome if you could add support for fatal. Otherwise please provide some guidance and I'll help implement it. Thanks!

@jupiterrosen
Copy link

@pablodenadai @samthor @khoanguyen-yang I'm also encountering this issue when integrating with solanaweb3js

@krisgrm
Copy link

krisgrm commented Aug 17, 2023

yers please i need it also

@eduardhasanaj
Copy link

eduardhasanaj commented Aug 19, 2023

@samthor can you please let us know what is the status of this feature request?

@eduardhasanaj eduardhasanaj linked a pull request Aug 20, 2023 that will close this issue
@eduardhasanaj
Copy link

Till the pull request is approved you can use my branch

npm install github:eduardhasanaj/fast-text-encoding#add-fatal-option-support

@samthor
Copy link
Owner

samthor commented Aug 21, 2023

The PR #29 doesn't cover all cases, i.e., it will not detect invalid UTF-8:

  • it only covers one failure mode of UTF-8 encoding (it doesn't check continuation bytes) for the fallback code
  • it doesn't support Node (Buffer does not throw, it generates U+FFFD characters for invalid input)

for folks on the thread, the two questions you need to ask are:

  • do you actually want UTF-8 validation, or do you just have some upstream library that expects fatal to be supported
  • why do you want this at all: modern Node and browsers do not need this polyfill (I wrote it in 2017!), and I'd go as far as saying including it is actively harmful.

@eduardhasanaj
Copy link

@samthor I agree that #29 does not cover all cases on detecting invalid utf-8 for the reasons you mentioned.

  1. Some upstream library expects fatal mode to be supported.
  2. I nedd this polyfill in React Native.

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 a pull request may close this issue.

6 participants