Encodings used in iconv-lite mismatch the WHATWG Encoding spec significantly

[ChALkeR]

Previously: #360, now reopened with more data
Refs: jsdom/whatwg-encoding#22

Note

If interop with WHATWG Encoding is a non-target, feel free to close this
Documenting the discrepancies would be helpful though

In this image, whatwg-encoding is what iconv-lite does (as that's a wrapper on top of iconv-lite, I did not create a separate column)

Spec used: https://encoding.spec.whatwg.org/

1. Half of single-byte encodings including windows-1252 don't match the spec and decode differently

   E.g., even for the most basic windows-1252 encoding:

   > require('iconv-lite').decode(Uint8Array.of(0x8d, 0x8f, 0x90), 'windows-1252')
   '���'
   > require('@exodus/bytes/encoding.js').legacyHookDecode(Uint8Array.of(0x8d, 0x8f, 0x90), 'windows-1252')
   '\x8D\x8F\x90'

   The latter behavior is correct, see the mapping from the Encoding spec

2. utf-8 is wrong when bundled. Because https://npmjs.com/buffer polyfill is wrong and iconv-lite uses that instead of a clean impl.

3. utf-16 is wrong because it doesn't produce well-formed strings

4. All of the multi-byte encodings don't match the decoders in the WHATWG Encoding spec

I can test iconv-lite separately further but I confirmed that all those discrepancies are also happening on pure iconv-lite

If interop is desired:

1. Fix all single-byte mappings (here they are: https://encoding.spec.whatwg.org/#legacy-single-byte-encodings)
2. Replace utf-8 decoder with a compliant one (global TextDecoder with ignoreBOM is usually fine unless you are using stream)
3. Replace utf-16 decoder with a compliant one (global TextDecoder with ignoreBOM is fine unless you are running in Node.js without ICU, where utf16-le is exposed but broken and utf-16be does not exist)
4. Adjust legacy multi-byte decoders to behave by the Encoding spec (and likely encoders too, the spec describes those too)

For some of that, you could check how I did it in https://github.com/ExodusOSS/bytes 🙃
Which also exposes utf8/utf16 encoders/decoders and single-byte/legacy multi-byte decoders, but I doubt you want to depend on that as it would increase the tables size 1.5x
Improving the approach here based on that impl could be nice though

[bjohansebas]

definitely it's a target, being compatible with the spec, something that would be awesome to fix for version 1 of this package

[ChALkeR]

See web-platform-tests/wpt#56892 for specific test cases

[bjohansebas]

Note that TextDecoder is now used for v1 (500da91. #316). I haven’t added tests for the cases you mention, and I’m not sure whether it was fixed by the specification, but I’ll keep following up on it.

[ChALkeR]

@bjohansebas Node.js TextDecoder is broken in ~all versions released in 2025
Using it without consistency checks will cause issues
See nodejs/node#60888, nodejs/node#55275 (comment), nodejs/node#61041

[bjohansebas]

@bjohansebas Node.js TextDecoder is broken in ~all versions released in 2025
Using it without consistency checks will cause issues

I’ve been reading through all the issues referenced here and in your repository. The question would be whether to leave the implementation as it is and treat it as a Node.js issue, waiting for them to eventually fix it, or to implement our own TextDecoder. Maybe we could create a separate package for that implementation so everyone can benefit from it. That could be good for Node.js as well, similar to how they provide the readable-stream package for the stream module, which can be used by packages that want to maintain stability without breaking across Node.js versions.

[ChALkeR]

or to implement our own TextDecoder

I already have that, it's the left column in the table 😉
See https://github.com/ExodusOSS/bytes#textencoder--textdecoder-polyfill

[ChALkeR]

For things not covered by the Encoding spec, I'll file separate issues

[ChALkeR]

I checked the upstream iconv code

The main issue is that libiconv mismatches WHATWG Encoding spec

E.g., this has replacements for Windows-1252:
https://gitweb.git.savannah.gnu.org/gitweb/?p=libiconv.git;a=blob;f=lib/cp1252.h;hb=refs/heads/master#l24

Encoding spec mapping doesn't, and maps all bytes: https://encoding.spec.whatwg.org/index-windows-1252.txt

This means that the lib will have to either significantly diverge from the Encoding spec, significantly diverge from iconv, or maintain both mappings and provide a mode switch option

Just on top of iconv / iconv mappings, it is impossible to implement TextDecoder correctly
Neither is it possible to implement libiconv-compatible mappings just on top of TextDecoder (without maintaining those)

It could be done for some encodings though (see the table)

[bjohansebas]

iconv uses an old version of libiconv, although checking locally and replacing it with a newer version (1.18) doesn’t change anything in terms of the encodings, it only adds a few new ones.

The idea is to be compatible with the specification, so I think we could bring in all those encodings from https://github.com/pillarjs/iconv-lite/blob/v1/generation/gen-dbcs.js
instead of depending on iconv (https://github.com/pillarjs/iconv-lite/blob/v1/generation/gen-sbcs.js).

What I mean is that I prefer to diverge from iconv and be compatible with the specification.

[ChALkeR]

@bjohansebas So, the decision is to diverge from libiconv in v1 and instead sync with the encoding spec?
Perhaps then the approach of https://npmjs.com/package/@exodus/bytes could be reused (see delta-coding and data reordering)
Or even some code exported/shared to not triple the size of mappings

I've done some work to reduce the bundle size of those mappings, but didn't add the ones that are not in the Encoding spec.
I could add the missing ones if there is a good source for them (like unicode.org) and am planning to check if those could be slimmed down further anyway

[bjohansebas]

Sorry for the delay, I went on vacation for a few days over the end of the year and I’m back now.

Yep, that’s the decision. Since we try to follow the specification, and we’re a package that’s used in browsers, not only in Node.js, following the spec is a priority and would bring better consistency across runtimes, avoiding exceptional or unique error cases in specific browsers or runtimes, Additionally, this would greatly benefit all the packages that depend on iconv-lite, which are quite a few.

I've done some work to reduce the bundle size of those mappings, but didn't add the ones that are not in the Encoding spec.
I could add the missing ones if there is a good source for them (like unicode.org) and am planning to check if those could be slimmed down further anyway

What if we let your package fully follow the specification, and have iconv-lite integrate those encodings that aren’t in the WHATWG specification, along with the spec fixes your package already provides? I’ve been reflecting on this during vacation, and maybe to avoid code duplication it could be a good idea to integrate your package as a dependency here. I haven’t analyzed it fully yet, but it could help avoid repeating code. Or, alternatively, we could also make an adaptation just for what affects TextDecoder, specifically for iconv-lite.

Any thoughts, @pillarjs/express-tc @pillarjs/iconv-lite-collaborators?