Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transient Purescript-Text-Encoding is overly burdensome. #20

Closed
AlexaDeWit opened this issue Dec 6, 2018 · 20 comments
Closed

Transient Purescript-Text-Encoding is overly burdensome. #20

AlexaDeWit opened this issue Dec 6, 2018 · 20 comments

Comments

@AlexaDeWit
Copy link
Contributor

The transient dependency on the text encoding library is there to provide a very specific use-case that is not general to all users of this library, and thus makes more sense to be removed. Ideally this could be a two stage process where first a new library is released providing the needed functionality, and then the functionality in this library can be removed, alongside a link and explanation that a new library now provides this behavior.

This issue is being created in response to #19

@AlexaDeWit
Copy link
Contributor Author

Oh crap, this is especially needed as this underlying library is now deprecated.

https://github.com/inexorabletash/text-encoding

In the deriving library I'll want to try and abstract this problem out.... which means back to my old nightmare.

@jacereda
Copy link
Collaborator

jacereda commented Dec 6, 2018

What if you restrict it to UTF-8? There's a small polyfill here that could serve as a starting point:

https://developer.mozilla.org/en-US/docs/Web/API/TextEncoder

@AlexaDeWit
Copy link
Contributor Author

I lack a minimalist isomorphism to do the decode easily available. I don't remember the full history of the work I was doing.

I'm also thinking of working further at erasing the need for a polyfill in the other library, but that is more of a long term goal.

@AlexaDeWit
Copy link
Contributor Author

I kind of agree with the "breakout" library idea actually, as I'm considering improving another library I maintain which has minimal behaviour for base64 encoding, and wrapping these dependencies in a library specifically for working with strings, base64, and arraybuffers collectively.

Did you want to preserve some semblance of the string encoding/decoding for this lib here?

@jacereda
Copy link
Collaborator

jacereda commented Dec 6, 2018

No problem on my side, I have only used binary arraybuffers myself.

@AlexaDeWit
Copy link
Contributor Author

If you were to release the stripped down version a la my PR here as another major version, I think I can commit to having a library up by the end of the weekend to provide the now missing functionality, and I could PR your Readme to include a note about what changed and how to resolve the problem.

What do you think?

Important to note I also removed the Show instance as it depended on a node-only inspect.

@jacereda
Copy link
Collaborator

jacereda commented Dec 6, 2018

Would merging to a temporary branch suffice? @athanclark will probably change the API as well and 2 major bumps could be a bit strange...

@AlexaDeWit
Copy link
Contributor Author

Sure, I can always do my next library using a local fork copied into the directory. Make a branch for me to merge into and I'll update the PR.

@jacereda
Copy link
Collaborator

jacereda commented Dec 7, 2018

I have created https://github.com/jacereda/purescript-arraybuffer/tree/v9 including your PR.

@AlexaDeWit
Copy link
Contributor Author

Cheers to that. So I assume we expect that @athanclark 's work will also end up here as part of the v9 release?

@AlexaDeWit
Copy link
Contributor Author

https://github.com/AlexaDeWit/purescript-arraybuffer-encoding This is where the new library will live btw.

@athanclark
Copy link
Collaborator

I'm right in the middle of thoroughly testing my branch - I'll make sure to pull whatever you've changed. I don't have any form of text-encoding integration right now - I ripped out everything, so any upgrades should be smooth, and I haven't touched arraybuffer-types.

@AlexaDeWit
Copy link
Contributor Author

Sounds good. My changes also rip out text-encoding completely, so you may have to resolve conflicts if we did it differently.

@jacereda
Copy link
Collaborator

jacereda commented Dec 7, 2018

@AlexaDeWit would -codecs be a better name since it will also have decoding?

@jacereda
Copy link
Collaborator

jacereda commented Dec 7, 2018

or text-codecs...

@AlexaDeWit
Copy link
Contributor Author

I'll give the name some thought. My intent also intends to include working with a wrapped base64 type (still text underneath ofc), but yeah, I'm not fully happy with the name either. If/When I rename though that link will still be valid for a while to find under the new name. Github rename redirects are cozy.

@AlexaDeWit
Copy link
Contributor Author

purescript-arraybuffer-codecs maybe, minus the "text" bit. Is decent.

@AlexaDeWit
Copy link
Contributor Author

https://github.com/AlexaDeWit/purescript-arraybuffer-codecs renamed, I'll die on this hill and all that. <3

@athanclark athanclark mentioned this issue Dec 15, 2018
@jacereda
Copy link
Collaborator

I think #23 will take a while.

I cherry-picked your changes to master and published v9.0.0. Thanks.

@jacereda
Copy link
Collaborator

jacereda commented Feb 1, 2019

The v10 branch has been merged to master, tell me if you have any problem with the new API in your -arraybuffer-codecs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants