Closed
Conversation
Member
|
Tough call, I have no strong opinion 🤷♂️ I'm a huge fan of conciseness but I also think bundlesize is not an issue for v3/5 and performance is a promise that this library gives. I would make good-enough test coverage the requirement for the complex fast solution (I haven't checked how good the test coverage is for this specific code path). In the next step we could also consider exposing this method alongside |
|
TBQH, the code in here is not entirely illegible, especially with the comments. If that is the only concern I would go with it for the small perf and size improvement. And also, 👍 to exposing it or moving it out to a separate package. Either one would have solved the issue I had in #180 |
Member
Author
|
Moving forward with #464 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Putting up two possible implementations of
uuidToBytesfor us to consider:uuidToBytes_concise, is similar to what's onmaster. Slightly better perf and compactness, pretty readable.uuidToBytes_fastis @awwit's approach inv9, but with thenumberToBytesstuff done inline. Yields slightly better performance and saves a few bytes when minified. Not great readability (as tends to be the case with highly optimized code), hence the extra comments.Here are the numbers for the various flavors:
master2v9uuidToBytes_conciseuuidToBytes_fastThoughts? Pick one, I guess. :-)
uuidToBytes()implementation run throughjsmin | gzip -c | wc -cmasterimplementation does not currentlyvalidate()like the other approaches mentioned here do.