remove use of this with TextDecoder
#348
Conversation
this fixes issues with ESM bundlers such as rollup
lib/utils.js
Outdated
| @@ -464,7 +464,7 @@ const decoders = { | |||
| if (typeof data === 'string') | |||
| data = Buffer.from(data, 'latin1'); | |||
| try { | |||
| const decoder = new TextDecoder(this); | |||
| const decoder = new TextDecoder(); | |||
There was a problem hiding this comment.
This isn't the correct change. Instead,
other: (data, hint) => {should be:
other: function(data, hint) {There was a problem hiding this comment.
Hmm, maybe we can make that a bit clearer? Something like:
other: (charset) => (data, hint) =>There was a problem hiding this comment.
I think using bind() will lead to better efficiency as V8 should be able to optimize better rather than returning a completely "different" function every time.
There was a problem hiding this comment.
Yes, it requires a change to the call site (in getDecoder) as well (see the latest commit). If you feel it has other issues I can revert to using bind.
There was a problem hiding this comment.
Because I am curious: https://jsbench.me/1ilnv78d75/1
Looks like .bind has some overhead and slightly loses out, at least on my machine haha. V8 must do some clever things to optimize a repetitive function creation.
this fixes issues with ESM bundlers such as rollup