Add OneOf property type annotation#146
Conversation
Code Size Check Report
Interface Check Report! WARNING this pull request has changed these public interfaces:
@@ -21229,9 +21229,9 @@
* @param type
*/
export function type(type: Function | [
Function
- ] | any): __private._cocos_core_data_decorators_utils__LegacyPropertyDecorator;
+ ] | OneOfPropertyType | any): __private._cocos_core_data_decorators_utils__LegacyPropertyDecorator;
export function type<T>(type: __private._cocos_core_data_utils_attribute__PrimitiveType<T> | [
__private._cocos_core_data_utils_attribute__PrimitiveType<T>
]): __private._cocos_core_data_decorators_utils__LegacyPropertyDecorator;
/**
@@ -21253,8 +21253,39 @@
* @en Declare the property as string
* @zh 将该属性标记为字符串。
*/
export const string: __private._cocos_core_data_decorators_utils__LegacyPropertyDecorator;
+ export function OneOf<T = unknown>(options: OneOfOptions<T>): OneOfPropertyType<T>;
+ export interface OneOfOptions<T = unknown> {
+ variants: readonly OneOfVariant<T>[];
+ discriminator?: OneOfDiscriminator<T>;
+ }
+ export type OneOfVariant<T = unknown> = __private._cocos_core_data_decorators_one_of__OneOfConstructor<T> | OneOfTypedVariant<T> | OneOfKeyVariant<T>;
+ export interface OneOfTypedVariant<T = unknown> {
+ type: __private._cocos_core_data_decorators_one_of__OneOfConstructor<T>;
+ label?: string;
+ key?: OneOfKey;
+ create?: () => __private._cocos_core_data_decorators_one_of__NoInferType<T>;
+ }
+ export interface OneOfKeyVariant<T = unknown> {
+ key: OneOfKey;
+ create: () => unknown;
+ label?: string;
+ type?: __private._cocos_core_data_decorators_one_of__OneOfConstructor<T>;
+ }
+ export type OneOfDiscriminator<T = unknown> = string | ((value: T) => OneOfKey) | {
+ kind?: "field";
+ property: string;
+ } | {
+ kind: "type-id";
+ };
+ export type OneOfKey = string | number | boolean | null;
+ export class OneOfPropertyType<T = unknown> {
+ readonly [__private._cocos_core_data_decorators_one_of__ONE_OF_BRAND]: true;
+ readonly discriminator: __private._cocos_core_data_decorators_one_of__NormalizedOneOfDiscriminator<T>;
+ readonly variants: readonly __private._cocos_core_data_decorators_one_of__NormalizedOneOfVariant<T>[];
+ constructor(options: OneOfOptions<T>);
+ }
}
export function CCClass<TFunction>(options: {
name?: string;
extends: null | (Function & {
@@ -65774,16 +65805,38 @@
* 该签名同时兼容 TypeScript legacy 装饰器以及 Babel legacy 装饰器。
* 第三个参数在 Babel 情况下，会传入 descriptor。对于一些被优化的引擎内部装饰器，会传入 initializer。
*/
export type _cocos_core_data_decorators_utils__LegacyPropertyDecorator = (target: Record<string, any>, propertyKey: string | symbol, descriptorOrInitializer?: _cocos_core_data_decorators_utils__BabelPropertyDecoratorDescriptor | _cocos_core_data_decorators_utils__Initializer | null) => void;
- export type _cocos_core_data_decorators_property__SimplePropertyType = Function | string | typeof CCString | typeof CCInteger | typeof CCBoolean;
+ export type _cocos_core_data_decorators_property__SimplePropertyType = Function | string | typeof CCString | typeof CCInteger | typeof CCBoolean | _decorator.OneOfPropertyType;
export type _cocos_core_data_decorators_property__PropertyType = _cocos_core_data_decorators_property__SimplePropertyType | _cocos_core_data_decorators_property__SimplePropertyType[];
export class _cocos_core_data_utils_attribute__PrimitiveType<T> {
name: string;
default: T;
constructor(name: string, defaultValue: T);
toString(): string;
}
+ export type _cocos_core_data_decorators_one_of__OneOfConstructor<T = unknown> = Function & {
+ prototype: T;
+ };
+ export type _cocos_core_data_decorators_one_of__NoInferType<T> = [
+ T
+ ][T extends unknown ? 0 : never];
+ export const _cocos_core_data_decorators_one_of__ONE_OF_BRAND = "__ccOneOfPropertyType__";
+ export type _cocos_core_data_decorators_one_of__NormalizedOneOfDiscriminator<T = unknown> = {
+ kind: "type-id";
+ } | {
+ kind: "field";
+ property: string;
+ } | {
+ kind: "function";
+ get: (value: T) => _decorator.OneOfKey;
+ };
+ export interface _cocos_core_data_decorators_one_of__NormalizedOneOfVariant<T = unknown> {
+ type?: _cocos_core_data_decorators_one_of__OneOfConstructor<T>;
+ label?: string;
+ key?: _decorator.OneOfKey;
+ create?: () => unknown;
+ }
namespace _cocos_core_data_utils_attribute {
export const DELIMETER = "$_$";
export function createAttrsSingle(owner: Object, superAttrs?: any): any;
/**
|
|
@shrinktofit, Please check the result of
Task Details |
|
@shrinktofit, Please check the result of
Task Details |
bofeng-song
left a comment
bofeng-song
left a comment
There was a problem hiding this comment.
Code Review
Reviewed across 7 dimensions (line-by-line scan, removed-behavior audit, cross-file tracing, reuse, simplification, efficiency, altitude). Findings ranked by severity below.
| @@ -80,7 +81,7 @@ export function attr (constructor: any, propertyName: string): { [attributeName: | |||
| ret[key.slice(prefix.length)] = attrs[key]; | |||
| } | |||
| } | |||
There was a problem hiding this comment.
🔴 Performance: applyDynamicOneOfAttrs on every attr() call — uncached object allocation + create() invocation
attr() is the engine's universal attribute accessor, called per-property per-frame by the Inspector, serialization, and deserialization.
For any OneOf-typed property where attrs.oneOf is truthy and owner is an instance, applyDynamicOneOfAttrs will:
- Spread
{ ...attrs }into a new object (allocation) - Clone all variant userData via
.map(item => ({ ...item }))— O(variants) allocations - Call
variant.create()to compute a default value — constructing a throwaway instance every time
None of this is cached. In a scene with many OneOf components, this causes GC pressure on every Inspector repaint.
Also, this breaks reference identity: callers that cache attr() results and compare by reference will see a new object every time, potentially causing infinite re-render loops in reactive systems.
Suggestion: Cache the dynamic attrs result (keyed on the current variant index), or at minimum cache the create() result so a new instance isn't constructed on every read.
There was a problem hiding this comment.
Addressed in d7ebee9. Dynamic attrs no longer call variant.create(), no longer synthesize defaults, and no longer clone or mutate userData.oneOf.variants. The dynamic path now only shallow-copies the attrs/userData wrapper to expose currentKey/currentVariantIndex and root type/ctor for the current value; tests assert the variants array keeps the same reference and create-only variants do not get inferred type metadata.
| (this as Record<string, unknown>)[propertyName] = createOneOfVariantValue(oneOfType, index); | ||
| }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
🔴 Bug: No serialization handler recognizes the 'OneOf' type tag
This sets attrs.type = 'OneOf' (via ONE_OF_TYPE_TAG). However, the engine's serialization/deserialization code only recognizes known type tags: 'Float', 'Integer', 'Boolean', 'String', 'Enum', 'BitMask', and constructor references.
applyDynamicOneOfAttrs dynamically rewrites the type for Inspector rendering, but deserialization reads static attribute metadata directly — it does not go through the dynamic rewrite path.
When a scene is saved and reloaded, the deserializer encounters type: 'OneOf' with no handler. Depending on the code path, it may skip the property (data loss) or fail silently.
Suggestion: Verify that OneOf properties survive a full serialize → deserialize round-trip. If the serializer relies on type to choose a strategy, consider keeping the concrete variant's constructor as the serialized type, and storing 'OneOf' only in userData.
There was a problem hiding this comment.
Addressed in d7ebee9 with coverage for the deserialization side. The static property metadata can remain type: OneOf because the saved value still carries the concrete variant object type. I added a regression where a OneOf property is saved with a concrete Cat object while the property metadata is OneOf, and deserialize restores the concrete Cat instance.
| ): Record<string, any> { | ||
| const oneOfUserData = userData.oneOf; | ||
| if (!oneOfUserData || typeof oneOfUserData !== 'object') { | ||
| return userData; |
There was a problem hiding this comment.
🔴 Bug: === strict equality fails across JSON serialization boundaries
oneOf.variants.findIndex((variant) => 'key' in variant && variant.key === key);If a variant is defined with key: 0 (number), but the discriminator field value went through JSON round-tripping (e.g., Inspector dump → JSON → parse), the value arrives as '0' (string). 0 === '0' is false.
Same issue for boolean keys: true === 'true' is false.
Result: findOneOfVariantByKey returns undefined, applyDynamicOneOfAttrs can't resolve the current variant, and the Inspector renders the property as Unknown with no editing capability.
Suggestion: Use loose comparison (==) for key matching, or coerce both sides to string before comparing:
oneOf.variants.findIndex((v) => 'key' in v && String(v.key) === String(key));There was a problem hiding this comment.
Leaving this strict intentionally. The discriminator result and variant key are both OneOfKey values, and coercing 0 and "0" would make distinct variants ambiguous. JSON preserves number/string/boolean/null value types here, so callers should keep the key type consistent with the discriminator output.
| if (!value || typeof value !== 'object') { | ||
| return undefined; | ||
| } | ||
|
|
There was a problem hiding this comment.
🔴 Bug: Unhandled exception in switch setter causes Inspector/value desync
The set handler calls createOneOfVariantValue(oneOfType, index) which may call new (variant.type)() with zero arguments. If the constructor requires mandatory parameters, it throws.
This set handler has no try/catch, so the exception propagates. But the Inspector has already updated the dropdown selection to the new variant. The actual property value remains unchanged.
Result: Inspector shows variant B selected, actual value is still variant A. The user sees no error.
Note: createOneOfVariantDefaultValue (line 389) has a try/catch for the attr() path, but this setter does not.
| set (value: unknown) { | |
| const index = getOneOfSwitchCommandIndex(value); | |
| if (index === undefined || index >= oneOfType.variants.length) { | |
| return; | |
| } | |
| try { | |
| (this as Record<string, unknown>)[propertyName] = createOneOfVariantValue(oneOfType, index); | |
| } catch (error) { | |
| console.warn(`[OneOf] Failed to create variant ${index} for property "${propertyName}":`, error); | |
| } |
There was a problem hiding this comment.
Addressed in d7ebee9. The hidden switch setter now catches create failures, keeps the previous property value, and emits a DEV warning. Added coverage for the failing create path.
| return; | ||
| } | ||
|
|
||
| const prototype = constructor.prototype as Record<string, unknown>; |
There was a problem hiding this comment.
🟡 Bug: Discriminator field assignment silently fails on frozen/readonly objects
After create() returns, this line assigns the discriminator key onto the value. If the create() factory returns a frozen object (Object.freeze(...)) or the discriminator field is defined with writable: false / readonly, the assignment silently fails in sloppy mode or throws in strict mode.
The value will have the wrong discriminator field, causing findCurrentOneOfVariant to match the wrong variant on the next read.
Suggestion: Add a guard or warning:
if (oneOfType.discriminator.kind === 'field' && 'key' in variant && value && typeof value === 'object') {
try {
(value as Record<string, OneOfKey>)[oneOfType.discriminator.property] = variant.key as OneOfKey;
} catch {
console.warn(`[OneOf] Cannot set discriminator "${oneOfType.discriminator.property}" on frozen/readonly object`);
}
}There was a problem hiding this comment.
Addressed in d7ebee9. createOneOfVariantValue no longer writes the discriminator field after construction; the variant factory/type is responsible for returning a value with the proper discriminator. Added coverage for getter-only field discriminators.
| return isOneOfKey(key) ? findOneOfVariantByKey(oneOf, key) : undefined; | ||
| } | ||
| case 'function': { | ||
| if (value == null) { |
There was a problem hiding this comment.
🟡 Silent error swallowing — create() factory failures produce no diagnostic
function createOneOfVariantDefaultValue (oneOfType, index) {
try {
return createOneOfVariantValue(oneOfType, index);
} catch (error) {
return undefined;
}
}This silently catches ALL exceptions from user-provided create() factories. When a factory throws due to a bug (missing dependency, initialization error), the property gets no default value, the Inspector shows a blank field, and there is zero diagnostic output.
Suggestion: At minimum log a warning in DEV mode:
| if (value == null) { | |
| function createOneOfVariantDefaultValue (oneOfType: OneOfPropertyType, index: number): unknown { | |
| try { | |
| return createOneOfVariantValue(oneOfType, index); | |
| } catch (error) { | |
| if (DEV) { | |
| console.warn(`[OneOf] Failed to create default value for variant ${index}:`, error); | |
| } | |
| return undefined; | |
| } | |
| } |
There was a problem hiding this comment.
Addressed in d7ebee9. Attribute lookup no longer calls create(), so there is no swallowed factory failure on read. The remaining factory invocation path is the switch setter, and that now reports a DEV warning while preserving the previous value.
| oneOfType: OneOfPropertyType, | ||
| propertyName: string, | ||
| ): IExposedAttributesUserData { | ||
| const merged = userData && typeof userData === 'object' && !Array.isArray(userData) |
There was a problem hiding this comment.
🟡 EDITOR guard mismatch: Inspector always dispatches to the hidden accessor
installOneOfSwitchVirtualAccessor is guarded by if (EDITOR || TEST), so the __cc_oneOfSwitch_<prop> setter only exists in editor/test builds.
However, the Inspector side (one-of-prop.js) unconditionally dispatches variant-switch commands to this accessor path via change-dump / confirm-dump events.
If any non-EDITOR build path reaches this code (preview mode, third-party editor integrations), the dump write will store the raw command string '__cc_oneof_switch__:N' as the property value instead of triggering the setter, silently corrupting component state.
Suggestion: Add a runtime guard in the setter dispatch, or document this constraint clearly.
There was a problem hiding this comment.
Keeping this as an editor/test-only bridge. one-of-prop.js lives in the editor inspector layer and dispatches the switch command only through that layer; the runtime API should not receive these command strings. I also kept the hidden accessor behind EDITOR || TEST and added inspector tests for switch/reset behavior.
| ? { kind: 'function' as const } | ||
| : oneOfType.discriminator; | ||
| return { | ||
| discriminator, |
There was a problem hiding this comment.
🟡 Side-effect: owner[propertyName] read on every attr() call
const value = owner[propertyName];For OneOf properties, every attr() call now reads the actual property value from the instance. If this property has a getter with side effects (logging, lazy initialization, state mutation), those side effects fire on every attribute lookup — not just when the user interacts with the property.
attr() was previously a pure metadata lookup with no instance access. This change makes it impure for OneOf properties.
Suggestion: Consider passing the current value explicitly from callers that already have it, rather than reading it inside attr().
There was a problem hiding this comment.
Keeping this read intentionally for the instance dynamic attrs path. attr(Class, prop) remains a static metadata lookup; attr(instance, prop) for OneOf must read the current value to compute currentKey/currentVariantIndex and the root type/ctor used by the Inspector. Passing the value explicitly would require a wider attr API change, so I am leaving this as the expected OneOf inspector behavior.
knoxHuang
left a comment
knoxHuang
left a comment
There was a problem hiding this comment.
I left a few focused OneOf review comments.
| return undefined; | ||
| } | ||
|
|
||
| const index = Number(value.slice(ONE_OF_SWITCH_COMMAND_PREFIX.length)); |
There was a problem hiding this comment.
[must fix] Empty switch commands are currently parsed as variant 0 because Number('') returns 0. If the Inspector has no resolvable current variant, the select value can be empty, and reset/change can silently switch the property to the first variant. Please reject empty suffixes explicitly, for example by validating the suffix with a decimal-integer check before converting it.
There was a problem hiding this comment.
Fixed in 31037b7. Empty and non-decimal switch suffixes are now rejected before Number() conversion, while index 0 remains valid. The editor dispatch path also uses the same non-negative decimal check so an unresolved empty select value no longer switches to variant 0. Added core parser coverage and inspector change/reset coverage for the unresolved case.
| const variantCtor = getOneOfVariantCtor(current.variant); | ||
| ctor = getOneOfValueCtor(value) | ||
| || (variantCtor && isOneOfValueOfCtor(value, variantCtor) ? variantCtor : undefined); | ||
| if (ctor) { |
There was a problem hiding this comment.
[must fix] For object variants this dynamic attr path sets ctor, but it leaves type as the static OneOf tag. Existing object-property attrs use type: 'Object' together with ctor, and render/dump/type-check code commonly branches on type first. Please also set nextAttrs.type = 'Object' when a concrete object ctor is resolved, and add a regression that asserts object OneOf attrs expose both type: 'Object' and the concrete ctor.
There was a problem hiding this comment.
Fixed in 31037b7. Dynamic object OneOf attrs now return type: Object with the concrete ctor, while userData.oneOf remains available for the selector. Added regression coverage for object-only and mixed object/primitive cases.
| return; | ||
| } | ||
|
|
||
| for (const variant of variants) { |
There was a problem hiding this comment.
[must fix] Discriminated variants should reject duplicate keys during validation. findOneOfVariantByKey() uses findIndex(), so if two variants share the same key, switching to the later one will resolve back to the first one on the next attr read and the Inspector selection/value will drift. Please track keys here and throw when a duplicate key is found.
There was a problem hiding this comment.
Fixed in 31037b7. Discriminated OneOf variants now validate duplicate keys during OneOf construction and throw before the invalid OneOf can be used. Added a regression for duplicate-key declarations.
| } | ||
| } | ||
| $prop.render(info); | ||
| const renderInfo = oneOfProp.normalizeOneOfDumpForRender(info); |
There was a problem hiding this comment.
[suggestion] This only wires OneOf normalization/decoration into the generic updatePropByDump() path. Existing custom inspector paths that render dump props through updateCustomPropElements() still call element.render(prop.dump) directly, so OneOf fields there will not get the selector or primitive normalization. Could we route both paths through a shared helper that normalizes the dump, renders it, and applies the OneOf decoration?
There was a problem hiding this comment.
Addressed in 31037b7. I added a shared propUtils.renderDumpProp() helper that normalizes the dump, renders it, and applies OneOf decoration. The generic updatePropByDump path now uses it, and the existing particle-system/widget custom prop paths call the same helper without changing their visibility/className flow. Added coverage for the shared renderer.
| */ | ||
| export function type (type: Function | [Function] | any): LegacyPropertyDecorator; | ||
| export function type ( | ||
| type: Function | [Function] | OneOfPropertyType | [OneOfPropertyType] | any, |
There was a problem hiding this comment.
[question] This public overload exposes [OneOfPropertyType], but I do not see support for switching individual array elements. preprocess-class collapses array type annotations to the element type, while the generated switch property/path still targets the root property. Should array-element OneOf be implemented in this PR, or should the array form stay out of the public type surface until that path is supported?
There was a problem hiding this comment.
Addressed in 31037b7 by narrowing the explicit public type surface. Array-element OneOf switching is not implemented in this PR, so I removed [OneOfPropertyType] from the type() overload/signature and keep only root-property OneOf as the declared supported form.
There was a problem hiding this comment.
Narrowing the type() overload helps, but the array form still leaks through property() because PropertyType = SimplePropertyType | SimplePropertyType[] and SimplePropertyType now includes OneOfPropertyType. That still allows @property([OneOf(...)]), while array-element OneOf switching is not implemented and preprocess-class still collapses array annotations to the element type.
If array-element OneOf is intentionally unsupported in this PR, could we also remove/reject the OneOf array form from the property() type surface, or emit a clear validation error for [OneOf(...)]?
|
@shrinktofit, Please check the result of
Task Details |
|
@shrinktofit, Please check the result of
Task Details |
4 participants
Re: # N/A
Changelog
OneOf(...)property type annotation support for polymorphic serialized fields and inline Inspector switching.Motivation
Some serialized properties naturally accept one value from several possible types. For example, a
shapeproperty may be a sphere, box, capsule, or torus; apet-like object may use a discriminator field such askind; and some less common fields may switch between primitive values or a small configuration object.Before this change, these cases usually had to be modeled with custom type selector properties or custom accessors. The setter would then create the selected variant manually and keep the actual serialized property in sync. This works, but each component has to reinvent the same pattern, and the Inspector UI cannot understand the relationship between the selector and the value being edited.
Idea
This PR introduces a
OneOf(...)property type annotation:It supports these variant shapes:
DemoSphere{ type: DemoBox, label: 'Box' }{ key: 'dog', create: () => new DemoDog(), label: 'Dog' }The current key can be resolved by:
discriminator: 'kind'(value) => typeof valueImplementation
The implementation keeps the existing property dump shape intact and uses
userData.oneOfmetadata to describe variants, labels, current key, and switch commands.On the engine side:
OneOfPropertyTypestores the normalized variants and discriminator.OneOf(...)as a special type annotation.type/ctor/ primitive type to existing property rendering.__cc_oneOfSwitch_<property>receives switch commands. This avoids adding a new dump mutation protocol; the Inspector can issue a normal property set request against the helper accessor, and the helper setter creates the selected variant and assigns it to the original property.On the Inspector side:
dump.userData.oneOf.type: Unknowndumps can still be rendered asNumber,String, orBooleanwhen the active runtime value is primitive.Pitfalls / Notes
create()may be called for switching and for type/default inference. It should be cheap and side-effect-free.Inspector Preview
The Inspector renders a selector inline with the current property row, then keeps the active variant's normal property renderer below it. For example:
Shapeis resolved by runtime type and displays object fields such asradiusandsegments.Petis resolved by thekinddiscriminator and displays labels where provided; variants without labels fall back to their key/type name.Tokendemonstrates primitive-only variants.Settingsdemonstrates a less common mixed union ofstring | boolean | object.Demo Code
Continuous Integration
This pull request:
Compatibility Check
This pull request: