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

Fix issues with enum options filtering logic in enum_ function #941

Merged
merged 7 commits into from
Jan 4, 2025

Conversation

ikeyan
Copy link
Contributor

@ikeyan ikeyan commented Nov 25, 2024

The current implementation of the enum_ function in Valibot filters enum options using the following code:

export function enum_(
enum__: Enum,
message?: ErrorMessage<EnumIssue>
): EnumSchema<Enum, ErrorMessage<EnumIssue> | undefined> {
const options = Object.entries(enum__)
.filter(([key]) => isNaN(+key))
.map(([, value]) => value);

The purpose of this filtering logic is to remove the reverse mapping entries that TypeScript adds when transpiling enums. Specifically, when an enum member is defined as Name = value and value is a number, TypeScript generates a reverse mapping in the form Enum[value] = 'Name'. The key of this reverse mapping is stringified by Number.prototype.toString with radix=10 via ToPropertyKey as defined by the ECMAScript specification.

There are several issues with using +key and isNaN in the filtering logic:

  • String Conversion: The +key operation follows the StringToNumber abstract operation as defined in the ECMAScript specification. Since enum_ filters out any number, this operation has several implications:

    • It trims leading and/or trailing whitespaces or line terminators.
    • If the string is empty after trimming, it returns 0.
    • Strings such as Infinity or -Infinity are interpreted as their respective number representations.
    • It interprets prefixes like 0b, 0o, or 0x as binary, octal, or hexadecimal literals.
    • Decimal variations such as 0.1, .1, and 0. are all valid and yield the respective numeric values.
    • It also allows leading zeros.
    • It accepts the + symbol as a valid sign.
  • NaN Values in Enum: If the enum contains NaN, as in the following example:

    enum N { X = NaN }
    // equivalent to js: const N = { X: NaN, NaN: 'X' }

    The isNaN check will not work as expected since isNaN(key) will return true when key is 'NaN', causing the 'NaN' key to be retained rather than filtered out.

To correctly filter out these reverse mappings, I made the filtering logic more precise. In this PR, rather than isNaN, the parsed value is stringified and is matched against the original key. If it matches, it is filtered out.

Additionally, the filtering logic is not reflected in the EnumSchema type. For example, when using enum_({ 1: "A" }), the options end up being empty, which was not obvious until runtime. Therefore I implemented type-level option filtering.

* filter out 'NaN'
* retain numeric strings that cannot occur in reverse mapping keys such as '1.0'
@fabian-hiller
Copy link
Owner

Thanks for creating this PR! Unfortunately I have almost no time to review and merge it in the next 3 weeks, but I plan to do it before we release our stable v1.

@fabian-hiller fabian-hiller self-assigned this Nov 25, 2024
@fabian-hiller fabian-hiller added priority This has priority fix A smaller enhancement or bug fix labels Nov 25, 2024
Copy link
Owner

@fabian-hiller fabian-hiller left a comment

Choose a reason for hiding this comment

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

Thank you for creating this PR! I look forward to your feedback to merge this PR soon.

@fabian-hiller
Copy link
Owner

I will improve the filtering of reverse mappings, undo the changes to types and merge this PR if you have no objections.

recognize reverse mapping entries by key and value
Copy link

pkg-pr-new bot commented Dec 28, 2024

Open in Stackblitz

npm i https://pkg.pr.new/valibot@941

commit: 79ee211

@fabian-hiller fabian-hiller force-pushed the main branch 2 times, most recently from f41426c to d713dfe Compare January 4, 2025 00:55
Copy link

vercel bot commented Jan 4, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
valibot ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 4, 2025 11:27pm

@fabian-hiller
Copy link
Owner

Thank you! 🧩

@fabian-hiller fabian-hiller merged commit 48a4ea3 into fabian-hiller:main Jan 4, 2025
13 checks passed
@fabian-hiller
Copy link
Owner

v1.0.0-beta.10 is available

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix A smaller enhancement or bug fix priority This has priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants