fix: non-english char encoding

[phawxby]

This closes #454 and works in a nice generic way by passing through an encoding param which is used to save the file. This required some changes to the afterResponse API to allow it to work correctly however via normalizeResponse it will retain compatibility. Now I just need to fix-up the tests

[phawxby]

@s0ph1e moving the conversation here. I've refactored a bit further this morning and I believe what I've got now is solid and retains compatibility with the existing API. However if people using it want it to work right then afterResponse handlers should be updated to return the modified response object rather than just the body. This way it can read the encoding from the incoming response to pass through to the resource it make sure it is written correctly.

[phawxby]

@s0ph1e for full transparency I think this PR might make #386 worse. utf8 representations of strings in memory are larger than binary representations. You get a more accurate result at the expense of memory.

console.log({
  utf8: Buffer.byteLength('Обладнання та ПЗ', 'utf8'),
  binary: Buffer.byteLength('Обладнання та ПЗ', 'binary'),
});

Result: { utf8: 30, binary: 16 }

[s0ph1e]

Hi @phawxby 👋

Thank you for your PR
I tested it locally and looks like everything works fine 💯 🎉
Few comments from my side:

• I suggest to remove string support for afterResponse - I think better to drop it in the next version instead of advising not to use it (or we can do it later, in next PR feat: improve memory performance and custom container classes #497 which definitely requires version bump). WDYT?
• There is a disabled test for encoding issue - could you please enable it?
• For some reason code coverage job fails - do you think it may be related to the changes? (on master it passes)

// Wow, happy to see Ukrainian here 🇺🇦 😉 
Buffer.byteLength('Обладнання та ПЗ', 'utf8') 

[s0ph1e]

If we bump major version for this change then we can get rid of string case here. That will simplify the code a lot

[aivus]

For some reason code coverage job fails - do you think it may be related to the changes? (on master it passes)

We can ignore this currently. It's GitHub security limitations.

[phawxby]

Hi @phawxby 👋

Thank you for your PR I tested it locally and looks like everything works fine 💯 🎉 Few comments from my side:

• I suggest to remove string support for afterResponse - I think better to drop it in the next version instead of advising not to use it (or we can do it later, in next PR feat: improve memory performance and custom container classes #497 which definitely requires version bump). WDYT?
• There is a disabled test for encoding issue - could you please enable it?
• For some reason code coverage job fails - do you think it may be related to the changes? (on master it passes)

// Wow, happy to see Ukrainian here 🇺🇦 😉 
Buffer.byteLength('Обладнання та ПЗ', 'utf8') 

No problem, this module has helped us in a pinch and mostly served our needs apart from these issues so was happy to contribute back to resolve them, Ukrainian characters were misbehaving and the string above was more problematic than most for some reason I didn't have time to investigate.

1. Happy to remove string support but perhaps we do it in feat: improve memory performance and custom container classes #497 to avoid a major version bump.
2. Done, I've also expanded the test and added some more elsewhere.
3. I don't know but it's suggested we can ignore it.

[s0ph1e]

Looks good to me, thank you @phawxby ✅ 🚀

Going to merge & release a new version today