-
-
Notifications
You must be signed in to change notification settings - Fork 13
feat: switch from iconv-lite to exodus/bytes #24
Conversation
|
Thanks for working on this! However, since it'd be semver-major anyway, I think it's best to just deprecate this package, and use exodus/bytes directly in the jsdom ecosystem. |
|
@domenic Yeah, that's ok |
This comment was marked as resolved.
This comment was marked as resolved.
ea2a8cb to
00690a4
Compare
|
Update:
|
00690a4 to
15cd210
Compare
|
@domenic I checked usage in It would be easier to keep lowercase -> cased mapping in a single place, at least while migrating And then perhaps switch to all-lowercase names directly? While semver-major, this is still a drop-in replacement Also there might be other usage in the ecosystem that would benefit from a switch to a fixed implementation, and it would be easier to do that without having to switch APIs |
They are? The encoding name concept is defined here https://encoding.spec.whatwg.org/#name and https://encoding.spec.whatwg.org/#names-and-labels is pretty clear that the names are cased, e.g. when it says
But anyway, I'm happy to move to lowercased names throughout the jsdom ecosystem. Although I prefer jsdom's style as it matches the standard better, it doesn't affect any important user-facing behavior, and the benefit of removing an abstraction layer is high. So, we can do a semver-major rev of html-encoding-sniffer to return the lowercased names instead of the canonical names, and to bump the engines requirements. The jsdom ecosystem treats semver major bumps as cheap so I'm not really worried about migration costs. |
Hm, true, that implies that name is a string! Other that that note, those are not exposed anywhere though and could be treated as enums. Also:
|
|
@domenic I'm adding some more tests to cover all known browser discrepancies (some of them are already fixed in Chrome and WebKit) and then planning to release a v1.0.0 of Are there any changes you want me to land before then? |
|
Overall it seems great! I guess maybe adding some documentation for those exports would be helpful, especially around your custom concept of "canonicalized encoding label" that your library is based around (i.e., what it uses instead of the spec's encoding names). But that's not a breaking change. I'm excited to use this to fix such long-standing issues in the jsdom ecosystem! |
|
@domenic I just published Added docs on hooks: https://github.com/exodusoss/bytes#exodusbytesencodingjs |
|
Closing all issues and PRs as this package is now deprecated. |
An experiment
Fixes: #22
Fixes: #23
(
iconv-liteBuffer usage maps to https://npmjs.com/buffer which has discrepancies)This is also faster and ~2x smaller than
iconv-lite.I kept the API compatible (including label names)
This brings in some differences though which might be a blocker for now or at least a reason for a semver-major:
^20.19.0 || >=22.13.0(from 18 currently supported by this module). 18 is EOL per Node.js release schedulereplacement, as it is expected to be supported in the hook for standards.Anything using this module to make a
TextDecoderpolyfill could get support forreplacementunexpectedly.Users should check that they are not using
replacementmanually.An alternative to be specifically block it from being used via this API.
I suggest to wait until an
1.0.0release before landing (hence a draft), but I wanted to file this as a place to get comments