fast-text-encoding icon indicating copy to clipboard operation
fast-text-encoding copied to clipboard

Published 20 hours ago • verified publisher icon samthor

Why `fatal` option is not supported?

Open khoanguyen-yang opened this issue 3 years ago • 10 comments

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

khoanguyen-yang avatar Nov 03 '22 13:11 khoanguyen-yang

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.

samthor avatar Nov 03 '22 23:11 samthor

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.

khoanguyen-yang avatar Nov 04 '22 01:11 khoanguyen-yang

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!

pablodenadai avatar May 23 '23 21:05 pablodenadai

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

jupiterrosen avatar Jul 15 '23 14:07 jupiterrosen

yers please i need it also

krisgrm avatar Aug 17 '23 08:08 krisgrm

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

eduardhasanaj avatar Aug 19 '23 17:08 eduardhasanaj

Till the pull request is approved you can use my branch

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

eduardhasanaj avatar Aug 20 '23 14:08 eduardhasanaj

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.

samthor avatar Aug 21 '23 23:08 samthor

@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.

eduardhasanaj avatar Aug 22 '23 05:08 eduardhasanaj