-
Notifications
You must be signed in to change notification settings - Fork 460
feat(emitter): generalize optional dictionary members to allow explicit undefined #2378
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
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for the PR! This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged. |
Bashamega
left a comment
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.
Please use KDL, and DO NOT us OverrideType
|
This should not be webcodecs specific but be a general emitter change. KDL should not be used either. |
I have updated this PR to address your feedback. I have shifted the implementation from a WebCodecs-specific fix to a generalized emitter change in src/build/emitter.ts. The generator now automatically appends | undefined to all optional dictionary members, ensuring consistency with how interface properties are handled. This approach eliminates the need for KDL patches or manual overrides in overridingTypes.jsonc, which I have cleaned up. I have also synchronized all baselines with the latest main branch and organized the work into two clean, functional commits. This change should correctly support exactOptionalPropertyTypes for all configuration dictionaries across the board. |
src/build/emitter.ts
Outdated
| let type = convertDomTypeToTsType(m); | ||
| if (!m.required && !type.split(" | ").includes("undefined")) { |
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.
This should probably pass a modified type to the conversion function rather than modifying the stringified result.
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.
I think I have satisfied this change too.
Now there are no more concatenations, but the management of | undefined, is handled natively, passing it directly to the conversion function.
Description
This PR updates the emitter logic to consistently handle optional dictionary members by explicitly allowing
undefinedin their type definitions. Following the maintainer's feedback, I have shifted from a specific fix forWebCodecsto a generalized emitter change, avoiding manual KDL patches or redundant entries inoverridingTypes.jsonc.Key Changes
src/build/emitter.tsto automatically append| undefinedto all optional dictionary members during the emission process.| undefinedis only added if not already present in the type union (e.g., preventing cases liketype?: undefined | undefined).| undefinedoverrides for dictionary members fromoverridingTypes.jsonc, as they are now handled natively by the generator.mainbranch to resolve conflicts and ensure consistency across all supported TypeScript versions.Why this is important?
prop?: T | undefinedpattern. This change brings Dictionaries (configuration objects) into alignment with that standard.exactOptionalPropertyTypes: In strict TypeScript configurations whereexactOptionalPropertyTypesis enabled, a property defined asprop?: Tcannot be explicitly assignedundefined. By changing this toprop?: T | undefined, we allow developers to explicitly passundefinedto configuration dictionaries, which is a common and often necessary pattern in Web APIs.undefinedis a valid value. This change accurately reflects those semantics in the generated TypeScript definitions.Implementation Details
The PR is organized into two functional commits:
feat(emitter): generalize optional dictionary members to allow explicit undefined: Contains the core logic changes in the generator.refactor(overrides): update baselines with generalized optional dictionary members: Contains the baseline updates, fully synced with the current state of the upstream repository.