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

feat: add base256emoji #187

Merged
merged 3 commits into from
Jun 23, 2022
Merged

feat: add base256emoji #187

merged 3 commits into from
Jun 23, 2022

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Jun 8, 2022

Ref: multiformats/multibase#88

Too much fun, I couldn't help myself. Not intended to be performant of course.

/cc @Jorropo

@rvagg rvagg requested a review from Gozala June 8, 2022 06:31
@rvagg rvagg force-pushed the rvagg/base256emoji branch from be1313d to a05e961 Compare June 8, 2022 06:33
Copy link

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand all the details of the implementation.

It seems like you made base256emoji a special cases which a bunch of adhoc rules.
I would try to avoid that since this is an increased load on the future maintainers (including future you).

@rvagg
Copy link
Member Author

rvagg commented Jun 8, 2022

There's two special cases in here:

  1. The this.baseDecode(text.slice(1)) normal-path which doesn't work for unicode strings, we just don't have a nice, efficient way to slice off the first multibyte unicode character in JS. So I'm special-casing it here so I can futz with Array.from().slice().join(), which is much more inefficient; but we're accepting that for this case. So without an efficient char slicing mechanism I'm fine with this. 🤷
  2. In the tests there's a special case because I'm not including base256emoji in the 'multiformats/basics' module that's a kitchen-sink export, so I have to add it back in where it's not there. That's maybe a line-ball call, perhaps it could be included in basics and that special-case would go away. Again, 🤷, maybe @Gozala has opinions on these though.

@Jorropo
Copy link

Jorropo commented Jun 8, 2022

we just don't have a nice, efficient way to slice off the first multibyte unicode character in JS

If JS doesn't have utf8.DecodeRuneInString then I guess it make sense. This path could be generalised if an other unicode base is made.

which is much more inefficient; but we're accepting that for this case

👍 this is clearly a negative priority item, being slow is fine

@rvagg
Copy link
Member Author

rvagg commented Jun 8, 2022

If JS doesn't have utf8.DecodeRuneInString

Well .. Array.from() will do a nice job, but the contract for decoders is that they currently accept a string with the first character lopped off. So I could use Array.from() on all of them and pass in an array of characters, and that would be really nice, but then this is a breaking change because we allow arbitrary external implementations of Multibase (I don't know if any exist, probably not, but the contract exists).

We could do that in a future release though, it would be nicer than a slice(1), you could then just iterate over a proper array rather than a string.

src/bases/base.js Outdated Show resolved Hide resolved
@rvagg rvagg requested a review from Gozala June 9, 2022 05:30
@Gozala
Copy link
Contributor

Gozala commented Jun 9, 2022

Well .. Array.from() will do a nice job, but the contract for decoders is that they currently accept a string with the first character lopped off. So I could use Array.from() on all of them and pass in an array of characters, and that would be really nice, but then this is a breaking change because we allow arbitrary external implementations of Multibase (I don't know if any exist, probably not, but the contract exists).

I think contract is that they pass in string without multicodec tag, so far that meant first character, but if tag is first emoji in the sequence it would mean snipping it off.

@rvagg
Copy link
Member Author

rvagg commented Jun 16, 2022

Updated with s/☉/🐉 from the latest spec change for this

@rvagg rvagg merged commit c6c5c46 into master Jun 23, 2022
@rvagg rvagg deleted the rvagg/base256emoji branch June 23, 2022 03:09
github-actions bot pushed a commit that referenced this pull request Jun 23, 2022
## [9.7.0](v9.6.5...v9.7.0) (2022-06-23)

### Features

* add base256emoji ([#187](#187)) ([c6c5c46](c6c5c46))

### Trivial Changes

* **no-release:** bump @types/node from 17.0.45 to 18.0.0 ([#188](#188)) ([99e94ed](99e94ed))
* **no-release:** bump actions/setup-node from 3.1.0 to 3.2.0 ([#182](#182)) ([86ec43d](86ec43d))
* **no-release:** bump actions/setup-node from 3.2.0 to 3.3.0 ([#186](#186)) ([712c1c4](712c1c4))
* **no-release:** fix typo implemnetation -> implementation ([#184](#184)) ([3d4ae50](3d4ae50))
@github-actions
Copy link

🎉 This PR is included in version 9.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants