Skip to content

Add support and test for invalid encoding input to decoder-browser#15

Open
danieledler wants to merge 5 commits intocalvinmetcalf:masterfrom
danieledler:master
Open

Add support and test for invalid encoding input to decoder-browser#15
danieledler wants to merge 5 commits intocalvinmetcalf:masterfrom
danieledler:master

Conversation

@danieledler
Copy link

  • Use String(encoding) to not throw on encoding.trim() when for example the encoding content is retrieved from reading a file forgetting to add utf8 encoding
  • Check with iconv-lite if encoding exist before using TextDecoder which can otherwise throw on providing bad content
  • Fix ASNI -> ANSI typo
  • Add browser test to also test decoder-browser

@calvinmetcalf
Copy link
Owner

ok so the issue is that IE is the only one that doesn't have StringDecoder so that's why we don't use iconv-lite in the browser because that would balloon the size of the bundle for just IE support the indexes can be required separately but I think I may have forgot to document that so I'd really prefer not to bring in iconv-lite to the main build, there is some good stuff in this pull though

if (!encoding) {
return defaultDecoder;
}
enconding = String(encoding).trim();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we just need to check if it's a buffer?

Copy link
Author

@danieledler danieledler Jan 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least the Uint8Array buffer in the new test is decoded properly in its toString method, but what can we do otherwise? Then we need to know the encoding of the encoding? 😆 The worst thing that could happen I guess is that we get something like "[object Buffer]" and TextDecoder throws because of invalid encoding label, but then we get the default decoder. Or do you think about something else?

I just saw that I made a typo in that very line though! 😳

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 this pull request may close these issues.

2 participants