dimabory / ecoji-js

JavaScript port of Ecoji
https://www.npmjs.com/package/ecoji-js
Apache License 2.0
5 stars 3 forks source link

Invalid rune provided: πŸ€„ #7

Open nival999 opened 5 years ago

nival999 commented 5 years ago

I keep getting the error "Invalid rune provided", but only for πŸ€„... Any idea how to fix this?

Also, separate issue quick fix, I am using binary data now after reading the issue about it, but it should be an option to set the encoding type for that part. There's no reason it should by default only decode binary data.... There should be a pass-inargument for that for both decode and encode. I tried to make a fork which implemented it but weirdly couldnt load the library any longer. So I ended up just converting all my data to binary data (difficult to find functions for it since Javascript didn't support binary data at all until only a few years ago).

dimabory commented 5 years ago

I keep getting the error "Invalid rune provided", but only for πŸ€„... Any idea how to fix this?

Please provide more info about what you try to do.

$ echo πŸ€„ | ecoji
πŸπŸŒ«πŸ†“πŸŠπŸ¦πŸ”¦πŸ“žπŸ‡ΊπŸŽ­πŸšΎπŸπŸŒ‡
nival999 commented 5 years ago

Sure. I'm running in the browser (Firefox) using browserify.

When I run this command: ecoji.decode("πŸ€„")

It an error "Expected more than 4 emojis"

When I run this command: ecoji.decode("πŸ€„πŸ€„πŸ€„πŸ€„πŸ€„")

I get: Error: Invalid rune provided: πŸ€„

biocrypto730 commented 5 years ago

Do we know why this would be happening? lol. The unicode hex converter converts it to 1F004.

The behavior also happens in node, server-side.

dimabory commented 5 years ago

@nival999 @projectoblio Please check now

I've changed an error "Expected more than 4 emojis". When you run ecoji.decode("πŸ€„πŸ€„πŸ€„πŸ€„πŸ€„") you will get "Unexpected emoji sequence provided."

biocrypto730 commented 5 years ago

But I only obtained the rune character by running emoji.encode in this library. So how could the library possibly be outputting a rune character after encoding if it can't decode it? I couldn't figure this out looking at the source. But this could be a major issue with the lib depending in the use case.

dimabory commented 5 years ago

As @nival999 mentioned in https://github.com/dimabory/ecoji-js/issues/7#issue-404462757 the issue is caused by binary data encoding.

biocrypto730 commented 5 years ago

^ Thanks, but I think this is the comment you meant to link to: https://github.com/dimabory/ecoji-js/issues/6#issuecomment-458887043

dimabory commented 5 years ago

^ Thanks, but I think this is the comment you meant to link to: #6 (comment)

As well as comment from https://github.com/dimabory/ecoji-js/issues/7#issue-404462757

So I ended up just converting all my data to binary data (difficult to find functions for it since Javascript didn't support binary data at all until only a few years ago).

biocrypto730 commented 5 years ago

If the problem is with Binary data, then it makes no sense to have this line inside Ecoji.ts:

const bufferIterator = Buffer.from(input.encode(), 'binary')

Why not allow the user to specify the input encoding, like @nival999 said? Defaulting to a non-functioning parameter makes no sense. This lib is a lot less useful if you require users to do that. I will try submitting a PR, sorry if this comment came off as negative

dimabory commented 5 years ago

If I leave Buffer.from without 'binary encoding it won't work as well. You can check by yourself if you wish.

biocrypto730 commented 5 years ago

Can you help me write a decodeType function for hex? I can't figure out how to make the test work:

https://github.com/dimabory/ecoji-js/pull/9

dimabory commented 5 years ago

@projectoblio Can you provide specific use-case of this PR #9 ? Btw, I can't approve you PR without tests.