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
Backport adraffy/ens-normalize@1.10.1 to ethers@5 #4577
base: v5.7
Are you sure you want to change the base?
Conversation
|
||
// given a list of codepoints | ||
// returns a list of lists, where emoji are a fully-qualified (as Array subclass) | ||
// eg. explode_cp("abc💩d") => [[61, 62, 63], Emoji[1F4A9, FE0F], [64]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No non-ASCII7 characters are allowed in the source code (applies to v6 as well as v5) as they can cause issues in some editors, over various transports, etc. It also helps prevent "trojan" code.
A few quick notes I've noticed in a few places:
There are a lot of changes that spook me every time I read over this PR. Is it possible to not include the nf component for this v5 change? Is there a way to mostly just update the base64-encoded data and add an extra function or so where necessary? Just to minimize the changes? I also need to analyze the size with this change a bit more... Do you have any info on the size difference in the |
I can make this change and remove the emoji from the comments, I was just trying to satisfy whatever TSC was saying. I 100% agree that the extension-less includes are insane. I have a hard time reasoning about TSC builds as everyone does something different. There were significant changes from your early port of normalize to the final, eg. script (group) and confusable logic. In general, I can't do much about the file size, as it's the only the solution to: With async import, which I assume is not allowed in v5, I could dynamically import significantly smaller norm libraries using runtime capability checks, but I'm unsure how to package code like that, when this is just a small library in a framework in someone else's project. You don't need the NF part but considering this is a legacy build that's going to deployed on aging platforms, it's literally the use-case for a system-independent version of Unicode NF. The issue is that the % of names impacted by NF divergences is small. My goal was to make it so I can replace this with the system shim: https://github.com/adraffy/ens-normalize.js/blob/main/src/nf-native.js The other changes just involve removing duplicated logic.
The rest of the code is exactly the same as my currently library except the arguments are typed to satisfy TSC. |
…ecode.ts file, replace nf with system nf
|
ens-normalize/
directory with almost 1:1 port w/typesSet
with{[x]: x}
and membership withset[x]
orx in set
Map
with{}
class Emoji
with extra wrappervalidate.ts
runsens_normalize()
throughtests.json
→ no errorsens-normalize()
-related calls innamehash.ts
, functionality unchangedmax
argument todnsEncode()