Skip to content
This repository has been archived by the owner on Sep 10, 2024. It is now read-only.

Issues with optional dependencies on Windows #53

Closed
BMitrovic17 opened this issue Feb 10, 2018 · 7 comments
Closed

Issues with optional dependencies on Windows #53

BMitrovic17 opened this issue Feb 10, 2018 · 7 comments

Comments

@BMitrovic17
Copy link

BMitrovic17 commented Feb 10, 2018

While both of those dependencies are optional they are very valuable for day to day usage imho.

Being that there are issues with installing node-icu-charset-detector and iconv on Windows I'd suggest/request that you switch to combo of iconv-lite and jschardet.

You can see about that switch on mooz/node-icu-charset-detector#32 (comment), @vBm mentioned it on official node-irc here -> martynsmith#490 (comment)

Another solution is to switch to iconv-lite and charset-detector as @davericher did on his fork of node-irc here -> funsocietyirc/node-irc@e44e0fb...master

In reality it would be the best if you two would make an unified fork being that both of you have patches that are quite nice.

Thanks :)

@Throne3d
Copy link
Owner

I'm not super interested in trying to merge another fork's changes into mine, especially since we've both made substantial changes to the code solely through linting and pulling out various functions. If someone else put in the effort and I could verify somehow that they had implemented the changes successfully, I think I'd be happy with that. (I've already put in a lot of effort into this project :( and it seems like 90% of it was fixing messes in the upstream.)

Making this work better on Windows seems like a good priority; I've had trouble doing that myself, as in #52. Moving to iconv-lite and jschardet could be a good idea, but I'd want to investigate both libraries before moving straight to them, and I'm unlikely to get to it for the next month due to various other obligations on my time. If someone else wants to get to it in the meantime, I may be able to review it, but otherwise I should get to it within the next two months? (I realize that's quite a long wait, but I figure it's better to be realistic about when I'm going to get to it than flake out.)

Thanks for bringing this to my attention.

@davericher
Copy link

@BMitrovic17 on a side note, I took a look at jschardet and decided to use it instead of charset-detect

@davericher
Copy link

@Throne3d iconv-lite is almost now recommended over iconv, not to mention significantly faster. and jschardet gets 1000 downloads a day, so I mean, we should be able to trust it. All in all the switch to these is fairly painless.

@BMitrovic17
Copy link
Author

@davericher That's great.

@Throne3d Hopefully it'll take you far less than two months :)

@Throne3d
Copy link
Owner

Alright! This should now be fixed, with 1582fb8. If you wouldn't mind testing it, that'd be great. :P (I have some tests in the code, as you can see changed in that commit, but I'm not super confident in them.)

@BMitrovic17
Copy link
Author

Took some time to do proper testing being that I didn't have access to my box due to only having mobile internet. Anyway, it's working ok. So Thanks for implementing it.

@Throne3d
Copy link
Owner

No problem! I've just released a new version, v0.9.0, which contains this change.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants