ensure support for Uint8Array #246
Conversation
| @@ -64,7 +65,7 @@ module.exports = function(config) { | |||
|
|
|||
| // start these browsers | |||
| // available browser launchers: https://npmjs.org/browse/keyword/karma-launcher | |||
| browsers: ['PhantomJS'], | |||
| browsers: ['ChromeHeadless'], | |||
There was a problem hiding this comment.
Not sure if use of PhantomJS is intentional or accidental. In case it's intentional I would love to learn the reasons. In case it was accidental - here is my rationale for switching.
PhantomJS is suspended for about 2 years already. When I was writing tests it was giving me all kinds of weird errors for Uint8Array. Probably the support there is limited. So I figured it's easier to switch to something more up-to-date and stable than figure out those errors.
There was a problem hiding this comment.
I agree, Chrome Headless might be better here. Thank you!
test/webpack/basic-test.js
Outdated
|
|
||
| const encodings = Object.keys(iconv.encodings) | ||
| encodings.forEach(function(encoding) { | ||
| if (['base64', 'hex', '_internal', '0'].indexOf(encoding) >= 0) { |
There was a problem hiding this comment.
Excluded temporarily base64 and hex since they don't seem to work even with buffers: #247
|
That's great work Fedor! I'm a bit busy currently, so expect maybe 1-2 days
delays before I can look deeper. At the high level I like this approach.
--
Alexander Shtuchkin
…On Mon, Jun 22, 2020 at 12:56 AM Fedor Nezhivoi ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In test/webpack/basic-test.js
<#246 (comment)>:
> @@ -36,6 +36,29 @@ describe("iconv-lite", function() {
var str = iconv.decode(buf, "utf8");
assert.equal(str, "💩");
});
+
+ it("supports passing Uint8Array to decode for all encodings", function() {
+ iconv.encode('', 'utf8'); // Load all encodings.
+
+ const encodings = Object.keys(iconv.encodings)
+ encodings.forEach(function(encoding) {
+ if (['base64', 'hex', '_internal', '0'].indexOf(encoding) >= 0) {
Excluded temporarily base64 and hex since they don't seem to work even
with buffers: #247 <#247>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#246 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEZKHKONMGN6DPPZDM7CZDRX3QA3ANCNFSM4OEF3A4Q>
.
|
| @@ -64,7 +65,7 @@ module.exports = function(config) { | |||
|
|
|||
| // start these browsers | |||
| // available browser launchers: https://npmjs.org/browse/keyword/karma-launcher | |||
| browsers: ['PhantomJS'], | |||
| browsers: ['ChromeHeadless'], | |||
There was a problem hiding this comment.
I agree, Chrome Headless might be better here. Thank you!
test/webpack/basic-test.js
Outdated
|
|
||
| var encodings = Object.keys(iconv.encodings) | ||
| encodings | ||
| .filter(encoding => !encoding.startsWith('_') && encoding !== '0') |
There was a problem hiding this comment.
nit: hmm I'm not sure why you're filtering out "0"? I don't see this key in encodings object..
| .filter(encoding => !encoding.startsWith('_') && encoding !== '0') | |
| .filter(encoding => !encoding.startsWith('_')) |
test/webpack/basic-test.js
Outdated
| var byteArray = []; | ||
| for (var i = 0; i < encoded.length; i++) { | ||
| byteArray[i] = encoded[i]; | ||
| } | ||
| var uint8Array = Uint8Array.from(byteArray); |
There was a problem hiding this comment.
nit: Uint8Array can be created directly from a buffer (I think)
| var byteArray = []; | |
| for (var i = 0; i < encoded.length; i++) { | |
| byteArray[i] = encoded[i]; | |
| } | |
| var uint8Array = Uint8Array.from(byteArray); | |
| var uint8Array = Uint8Array.from(encoded); |
encodings/internal.js
Outdated
| if (Buffer.isBuffer(buf)) { | ||
| return this.decoder.write(buf); | ||
| } | ||
|
|
||
| return this.decoder.write(Buffer.from(buf)); |
There was a problem hiding this comment.
Yeah it seems internal encoder requires conversion to Buffer-s. A bit sad, but understandable, as otherwise they'd have to replicate all encoding/decoding logic from Buffer.toString.
nit: maybe structure it like this to highlight that we're just converting it into a buffer?
| if (Buffer.isBuffer(buf)) { | |
| return this.decoder.write(buf); | |
| } | |
| return this.decoder.write(Buffer.from(buf)); | |
| if (!Buffer.isBuffer(buf)) { | |
| buf = Buffer.from(buf); | |
| } | |
| return this.decoder.write(buf); |
| // Support Uint8Array | ||
| if (!Buffer.isBuffer(buf)) { | ||
| buf = Buffer.from(buf) | ||
| } |
There was a problem hiding this comment.
Ultimately I'd want to get rid of Buffer.concat so that we can avoid conversion to Buffers at all. Meanwhile, I think this stop gap is fine. (Same with utf32).
|
@ashtuchkin thank you for the review! All the changes applied. Would you be able to publish it as a new version, so I can continue with vscode? :) |
|
Yep, will publish in a couple mins |
|
ok, it's published as 0.6.1 |
I went ahead and created this PR for microsoft/vscode#79275 (comment) so you don't need to spend your time if you agree with the idea.
No pressure though, feel free to close it if you disagree 😄