Support parse method result #2485
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes enhance the resource deserialization process in the loader package by modifying the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/1.4 #2485 +/- ##
===========================================
- Coverage 68.42% 68.40% -0.02%
===========================================
Files 923 923
Lines 95904 96047 +143
Branches 8138 8136 -2
===========================================
+ Hits 65621 65704 +83
- Misses 30029 30089 +60
Partials 254 254
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts (1)
181-183: Recommendation for stronger type enforcement
Currently,_isRealClasschecks thatclassRealis not undefined. For more robust type checking, consider also verifying thatclassRealis a string.- return value["classReal"] !== undefined; + return typeof value["classReal"] === "string";packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts (1)
81-85: Consider documenting callbackProps usage
ThecallbackPropsfield significantly impacts parsing logic. A short comment or docstring explaining the purpose and format ofcallbackPropswould improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts(3 hunks)packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts(2 hunks)
🔇 Additional comments (3)
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts (2)
101-103: Confirm return value for real class references
Here, returningLoader.getClass(value["classReal"])only yields the constructor rather than an instantiated object. Verify that the calling code intends to handle the class constructor and not an instance. If you need an instance, consider callingnewon the constructor.
6-7: Nice addition for type imports
The introduction ofIClassRealObjectis consistent with the new schema logic. Please verify thatIComponentRefis indeed utilized in this file to avoid importing unused types.✅ Verification successful
IComponentReftype is actively used in the codebaseThe type
IComponentRefis properly utilized within theReflectionParserclass through:
- A type guard method
_isComponentRefthat checks if a value conforms to theIComponentRefinterface- The type is used in the method's return type annotation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check references to "IComponentRef" in this file rg "IComponentRef" packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts -A 5Length of output: 355
packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts (1)
94-94: Great addition to IBasicType
Extending the union withIClassRealObjectis consistent with the updated parser logic.
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts (4)
80-81: Remove redundant optional chainingThe optional chaining on
firstParam?.callbackPropsis redundant since we've already validatedfirstParamin theifcondition.- const callbackProps = firstParam?.callbackProps; + const callbackProps = firstParam.callbackProps;
82-94: Simplify promise handling and add error handlingThe promise chain could be simplified and should include error handling for invalid callback properties.
Consider this refactoring:
- const promises = []; - for (let key in callbackProps) { - const value = callbackProps[key]; - const promise = this.parseBasicType(value, methodCallback[key]).then((v) => { - if (methodCallback[key] !== v) { - methodCallback[key] = v; - } - return v; - }); - promises.push(promise); - } - return Promise.all(promises); + return Promise.all( + Object.entries(callbackProps).map(([key, value]) => { + if (!(key in methodCallback)) { + console.warn(`Callback property "${key}" not found in method result`); + return Promise.resolve(); + } + return this.parseBasicType(value, methodCallback[key]) + .then(v => { + if (methodCallback[key] !== v) { + methodCallback[key] = v; + } + }) + .catch(error => { + console.error(`Failed to parse callback property "${key}":`, error); + }); + }) + ).then(() => methodCallback);
105-107: Add validation for class resolutionThe class resolution could fail if the class name is invalid. Consider adding validation.
if (ReflectionParser._isRealClass(value)) { - return Promise.resolve(Loader.getClass(value["classReal"])); + const className = value["classReal"]; + const Class = Loader.getClass(className); + if (!Class) { + return Promise.reject(new Error(`Failed to resolve class "${className}"`)); + } + return Promise.resolve(Class); }
185-187: Strengthen type guard implementationThe type guard could be more robust by checking the type of the "classReal" property.
private static _isRealClass(value: any): value is IClassObject { - return value["classReal"] !== undefined; + return value && typeof value === "object" && typeof value["classReal"] === "string"; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts(3 hunks)
🔇 Additional comments (2)
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts (2)
6-7: LGTM! Type imports align with the new functionality.The addition of
IClassRealObjectandIComponentReftypes supports the new callback properties parsing feature.
76-97: Verify callback props usage and backwards compatibilityLet's verify the usage of callback props and ensure backwards compatibility.
✅ Verification successful
Let me gather more information about the callback props implementation to ensure there are no breaking changes.
Let me do one final check to understand the usage pattern and potential impact.
Callback props implementation appears safe and isolated
Based on the verification results, the callback props feature is a new addition that:
- Is properly defined in the schema (
BasicSchema.ts) as an optional property- Is only used within the
ReflectionParserclass for method parameter processing- Has minimal integration points (only
SceneLoaderusesReflectionParser)- Maintains backward compatibility since:
callbackPropsis optional in the schema- The implementation preserves existing behavior when
callbackPropsis not present- No existing code patterns were found that would be affected by this change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing method calls that might be affected echo "Searching for method calls that might need updates..." rg -l "addEffect|addShape" --type ts # Search for potential callback props usage echo "Searching for callback props usage..." ast-grep --pattern 'callbackProps: { $$$' --lang ts # Search for tests covering this functionality echo "Searching for related tests..." fd -e test.ts -e spec.ts | xargs rg "callbackProps|addEffect|addShape"Length of output: 9832
Script:
#!/bin/bash # Search for the ReflectionParser class implementation and usage echo "Searching for ReflectionParser implementation..." rg -A 5 "class ReflectionParser" --type ts # Search for method parameters with callback properties echo "Searching for method parameters with callback properties..." ast-grep --pattern 'interface $_Props { $$$ callbackProps?: { $$$ } $$$ }' --lang ts # Search for files importing ReflectionParser echo "Searching for ReflectionParser usage..." rg "import.*ReflectionParser" --type tsLength of output: 1501
Script:
#!/bin/bash # Check SceneLoader implementation for ReflectionParser usage echo "Checking SceneLoader implementation..." rg -A 10 "ReflectionParser" packages/loader/src/SceneLoader.ts # Look for any existing callbackProps usage in the codebase echo "Searching for callbackProps usage in the codebase..." rg "callbackProps" --type ts -A 3 # Check for any related tests echo "Looking for related parser tests..." fd "parser|reflection" -e test.ts -e spec.ts | xargs catLength of output: 2664
| const firstParam = methodParams?.[0] as IClassRealObject; | ||
| if (ReflectionParser._isRealClass(firstParam)) { |
There was a problem hiding this comment.
Unsafe type assertion needs validation
The type assertion as IClassRealObject is unsafe. Consider validating the type before the assertion.
Apply this diff to add proper type validation:
- const firstParam = methodParams?.[0] as IClassRealObject;
- if (ReflectionParser._isRealClass(firstParam)) {
+ const firstParam = methodParams?.[0];
+ if (firstParam && typeof firstParam === 'object' && ReflectionParser._isRealClass(firstParam)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const firstParam = methodParams?.[0] as IClassRealObject; | |
| if (ReflectionParser._isRealClass(firstParam)) { | |
| const firstParam = methodParams?.[0]; | |
| if (firstParam && typeof firstParam === 'object' && ReflectionParser._isRealClass(firstParam)) { |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts (1)
78-81:⚠️ Potential issueUnsafe type assertion needs validation.
The type assertion
as IClassRealObjectis unsafe. Consider validating the type before the assertion.Apply this diff to add proper type validation:
- const firstParam = methodParams?.[0] as IClassRealObject; - const callbackProps = firstParam?.callbackProps; - if (firstParam && ReflectionParser._isRealClass(firstParam) && callbackProps) { + const firstParam = methodParams?.[0]; + if (firstParam && typeof firstParam === 'object' && ReflectionParser._isRealClass(firstParam)) { + const callbackProps = firstParam.callbackProps; + if (callbackProps) {
🧹 Nitpick comments (2)
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts (2)
104-106: Add error handling for class loading.The class loading logic should handle cases where the class is not found.
Apply this diff to add error handling:
if (ReflectionParser._isRealClass(value)) { - return Promise.resolve(Loader.getClass(value["classReal"])); + const className = value["classReal"]; + try { + const LoadedClass = Loader.getClass(className); + if (!LoadedClass) { + throw new Error(`Class "${className}" not found`); + } + return Promise.resolve(LoadedClass); + } catch (error) { + return Promise.reject(new Error(`Failed to load class "${className}": ${error.message}`)); + } }
184-186: Strengthen type guard implementation.The current implementation only checks for property existence. Consider adding stronger type checking.
Apply this diff to improve type safety:
private static _isRealClass(value: any): value is IClassObject { - return value["classReal"] !== undefined; + return ( + value !== null && + typeof value === "object" && + typeof value["classReal"] === "string" && + value["classReal"].length > 0 + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts(3 hunks)
🔇 Additional comments (2)
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts (2)
6-7: LGTM! Import statements align with new functionality.The addition of
IClassRealObjectandIComponentReftypes supports the new callback properties feature.
77-78: TODO comment needs clarification or removal.The TODO comment about parsing only the first parameter needs to be either addressed or documented with a clear explanation of why this limitation exists.
Would you like me to help document this limitation or explore supporting multiple parameters?
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts (1)
31-37: LGTM! Consider adding JSDoc comments.The
ICanCallbackMethodObjecttype definition is well-structured and provides good type safety for callback scenarios. Consider adding JSDoc comments to document the purpose and usage of this type.+/** + * Represents a method object that can have callback functionality. + * @property params - Array of parameters for the initial method call + * @property callback - Optional callback configuration for method chaining + */ export type ICanCallbackMethodObject = { params: Array<IBasicType>; callback?: { methods?: { [methodName: string]: Array<IMethodParams> }; props?: { [key: string]: IBasicType | IMethodParams }; }; };packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts (1)
189-191: Strengthen type guard implementation.The current implementation only checks if
paramsexists and is an array. Consider adding more thorough validation.Apply this diff to strengthen the type guard:
private static _isCanCallbackMethodObject(value: any): value is ICanCallbackMethodObject { - return Array.isArray(value?.params); + if (!value || typeof value !== 'object') return false; + if (!Array.isArray(value.params)) return false; + if (value.callback !== undefined && typeof value.callback !== 'object') return false; + return true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts(4 hunks)packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts(3 hunks)
🔇 Additional comments (4)
packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts (3)
84-84: LGTM! Type restriction improves clarity.The change from
IMethodParamstoArray<IBasicType>forconstructParamsis a good improvement. It simplifies the type definition and better reflects that constructor parameters should be basic values rather than complex callback objects.
89-91: LGTM! Clean type system extension.The addition of
IClassTypeObjectand its integration intoIBasicTypeis well-designed. It provides a clean way to reference class types directly.Also applies to: 101-101
39-39: Verify type inference with circular reference.The type definition creates a circular reference:
IMethodParamsincludesICanCallbackMethodObjectwhich includesIMethodParams. While TypeScript can handle this, it might cause issues with type inference in complex scenarios.✅ Verification successful
No type inference issues found with the circular reference
Based on the code analysis, while there is indeed a circular reference between
IMethodParamsandICanCallbackMethodObject, it's a well-structured and valid TypeScript pattern:
IMethodParamsis a union type:Array<IBasicType> | ICanCallbackMethodObjectICanCallbackMethodObjectusesIMethodParamsonly within optional nested properties (callback.methodsandcallback.props)- The usage in
ReflectionParser.tsshows proper type handling with type guards (_isCanCallbackMethodObject)This is a common pattern in TypeScript for handling recursive structures (like ASTs or nested configurations), and TypeScript is designed to handle such cases. The circular reference is not problematic because:
- It's properly guarded by optional properties
- The base case (
Array<IBasicType>) provides a termination point- The actual usage shows proper type discrimination
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential type inference issues ast-grep --pattern 'IMethodParams' | grep -B2 -A2 'IMethodParams'Length of output: 161
Script:
#!/bin/bash # Let's check the definition and usage of ICanCallbackMethodObject ast-grep --pattern 'interface ICanCallbackMethodObject' # Also check where IMethodParams is used rg 'IMethodParams' -A 3 -B 3Length of output: 5185
Script:
#!/bin/bash # Let's check for IBasicType definition to complete our understanding ast-grep --pattern 'type IBasicType' # Also check if there are any actual usages that might show type inference issues rg -B2 -A2 '\bIMethodParams\b.*=.*ICanCallbackMethodObject'Length of output: 576
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts (1)
93-95: LGTM! Clean implementation of class type handling.The addition of
_isClassTypecheck and the handling of class type references is well-implemented. The order of checks is logical, checking for class types before class instances.
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts
Outdated
Show resolved
Hide resolved
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts
Outdated
Show resolved
Hide resolved
| methods?: { [methodName: string]: Array<IMethodParams> }; | ||
| props?: { [key: string]: IBasicType | IMethodParams }; | ||
| }; | ||
|
|
||
| export type IClassTypeObject = { |
There was a problem hiding this comment.
Need discuss: merge to IClassObject use needInstance to distinguish?
|
Why not use a second parameter of |
zhuxudong
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts (3)
31-37: Ensure thatICanParseResultObjectis well-documented.
This new interface is a crucial addition for parsing and handling callback results. Consider adding inline JSDoc comments describing the purpose ofparamsandresultfor enhanced maintainability.
84-84: Consider deprecating theconstructParamsproperty in the future.
SinceconstructParamsdiffers fromIMethodParams, you might consider deprecating or merging them if usage converges. This can simplify code and type definitions.
89-91: Address potential overlap withIClassObject.
We haveIClassTypeObjectrepresenting a “classType” andIClassObjectrepresenting a “class” string. Consider consolidating them or clarifying their differences if they serve overlapping roles.packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts (5)
5-8: Add usage context for the newly imported types.
The new imports (ICanParseResultObject,IClassTypeObject,IMethodParams) are critical to the parsing process. Adding a brief comment or docstring indicating how these types fit into the parser’s workflow may improve clarity for other contributors.
12-12: Consider renaming the import
IRefEntitymight be more intuitively named for better context, e.g.,IPrefabRefEntity, if that reflects its usage more accurately. Evaluate if the rename reduces confusion.
94-96: Document the purpose of_isClassType.
The_isClassTypemethod ensures that objects withclassTypeproperties are recognized correctly. Documenting the difference from_isClasshelps avoid confusion among maintainers.
174-176: Use consistent naming for_isClassType.
We have_isClassand_isClassType. While they serve different roles, consider standardizing function names for clarity, e.g.,_isClassByNamevs._isClassByType.
190-192: Improve null checks in_isCanParseResultObject.
Strictly check forvalue?.paramsin a type-safe manner. The current check is fine, but you might add an explicittypeof value === "object"guard for safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts(4 hunks)packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: e2e (22.x)
- GitHub Check: codecov
🔇 Additional comments (3)
packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts (2)
39-39: Highlight the reasoning behind updatingIMethodParams.
ExpandingIMethodParamsto acceptICanParseResultObjectcan introduce more flexible parsing logic. It’s recommended to add explanatory comments or usage examples to clarify when to use an array vs. theICanParseResultObject.
101-101: Confirm the union type ordering.
In some TypeScript versions, union type resolution might behave differently whenIMethodParamsreferences itself. Ensure there’s no circular reference or type confusion by verifying thatIBasicTyperemains consistently distinguishable.packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts (1)
76-86:⚠️ Potential issueValidate callable methods before
instance[methodName](...).
Even though this code is presumably safe, we should confirm thatinstance[methodName]is indeed callable (function). Otherwise, an unhelpful runtime error might occur.A minimal fix could be:
+ if (typeof instance[methodName] !== 'function') { + throw new Error(\`Method \${methodName} is not callable.\`); + } const methodResult = instance[methodName](...result);Likely invalid or redundant comment.
| @@ -28,7 +28,15 @@ export interface IHierarchyFile { | |||
| entities: Array<IEntity>; | |||
| } | |||
|
|
|||
| export type IMethodParams = Array<IBasicType>; | |||
| export type ICanParseResultObject = { | |||
There was a problem hiding this comment.
How about:
type IMethodParams = Array<IBasicType> ;
type INeedAssignInstance = {
methods?: { [methodName: string]: Array<IMethodParams> };
props?: { [key: string]: IBasicType | IMethodParams };
}
IMethodParamsAndRes = {
params: IMethodParams;
result?: INeedAssignInstance
}
type IMethodInfo = IMethodParams | IMethodParamsAndRes
There was a problem hiding this comment.
Use markdown and pay attention to format of typescript code.
| export type IMethodParams = Array<IBasicType>; | ||
| export type ICanParseResultObject = { | ||
| params: Array<IBasicType>; | ||
| result?: { |
There was a problem hiding this comment.
The problem is the addEffect method. Using a result to invoke functions or assign properties is not appropriate. You should follow the design of addComponent, which @cptbtptpbcptdtptp can understand.
There was a problem hiding this comment.
addComponents is handled specifically，addEffect can't follow it
There was a problem hiding this comment.
Can you give a specific example?
There was a problem hiding this comment.
我说的是HierarcyParser里的parseComponent，如果你指的是引擎代码里的addComponent可以加参数的形式的话，是不满足我们这次的需求的。addComponent只能传构造参数，且不能调用方法。这次需要addEffect调用之后拿到实例再去赋值和调用方法。
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/loader/src/SceneLoader.ts (1)
149-149: Code style: Replace line breaks per ESLint recommendation
ESLint/prettier suggests removing unnecessary line breaks around"TextRenderer". Consider updating the formatting to match the style guidelines.Apply this diff:
-ReflectionParser.registerCustomParseComponent( - "TextRenderer", - async (instance: any, item: Omit<IClass, "class">) => { +ReflectionParser.registerCustomParseComponent("TextRenderer", async (instance: any, item: Omit<IClass, "class">) => {🧰 Tools
🪛 eslint
[error] 147-149: Replace
⏎··"TextRenderer",⏎·with"TextRenderer",(prettier/prettier)
packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts (1)
82-82: Add precaution about wide usage
IInstance.methodscan become complex to manage in large-scale projects. Suggest providing helper utilities or strong validations to avoid misuse or confusion.packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts (2)
48-48: Extended property parsing
parsePropsAndMethodsworks well for dynamic property assignment, but do confirm that unknown or private fields do not create hidden side effects, especially in advanced user components that may use private symbols.
174-176: Simple & direct
_isClassTypereturning true ifclassTypeexists is straightforward. Consider checking for type ofclassTypeto guard against strings that might not be actual class references.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/loader/src/SceneLoader.ts(2 hunks)packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts(5 hunks)packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts(4 hunks)
🧰 Additional context used
🪛 eslint
packages/loader/src/SceneLoader.ts
[error] 147-149: Replace ⏎··"TextRenderer",⏎· with "TextRenderer",
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (22.x, windows-latest)
- GitHub Check: e2e (22.x)
- GitHub Check: codecov
🔇 Additional comments (17)
packages/loader/src/SceneLoader.ts (1)
17-17: Use updated type references consistently
The replacement ofIClassObjectwithIClassaligns well with the new schema definitions. Make sure that any remaining references toIClassObjectare removed and that new references toIClassare consistently applied across the codebase.packages/loader/src/resource-deserialize/resources/schema/BasicSchema.ts (10)
31-35: Clarify method structure
The newIMethodinterface (withparamsand optionalresult) is clear. Ensure that any code relying on the old structure (IMethodParamsas an array) properly checks for theIMethodobject shape now.
36-36: Good enhancement for method flexibility
AllowingIMethodParamsto be either an array of basic types or a fullIMethodobject increases flexibility. This is an excellent improvement as long as its usage is well documented for maintainers.
57-59: Preserve clarity in prefab modifications
The new(IInstance & { target: IPrefabModifyTarget })[]type clarifies the modifications array. Verify that all overrides or expansions of these modifications are tested to prevent runtime errors.
75-75: Accurate pairing:IComponent&IClass
CombiningIClasswith the component definition (idandrefId) is logical. This ensures every component is recognized as a class. Looks good.
77-77: Renamed type fosters clarity
RenamingIClassObjecttoIClassis consistent with the design. This minor improvement aids readability and better matches the conceptual usage of “class.”
79-80: Constructor parameter type
ChangingconstructParamsto a plainArray<IBasicType>clarifies how constructor arguments are expected. Validate usage to ensure no code still expects the olderIMethodParams.
85-85: Props usage
Allowing each prop to be either a singleIBasicTypeor anIMethodParamsfosters dynamic behavior but can obscure code clarity. Encourage thorough test coverage for complex scenarios.
86-86: Ensure no regressions
Although no immediate concerns are visible, keep an eye on performance when handling deeply nested props in large scenes.
87-88: Efficient handling ofIClassType
The addition ofIClassTypewithclassTypeis straightforward. Checking forclassTypeto identify specialized classes is helpful.
98-99: ExpandedIBasicType
AddingIClassandIClassTypetoIBasicTypecan increase complexity. Both the parser and intermediate modules should strongly validate these expanded possibilities to avoid tricky bugs.packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts (6)
5-7: Synchronize type usage
ImportingIClass,IClassType, andIComponentRefis consistent with the new definitions inBasicSchema. Confirm that any references to removed types (IClassObject) are fully purged across the project.
11-12: Seamless method integration
ImportingIMethodandIMethodParamsaligns with the newly introduced method-parsing approach. The code will be more flexible as a result.
40-40: Check usage with specialized class constructors
The updatedparseClassObject(item: IClass)might encounter classes with atypical constructors. Ensure you handle potential runtime issues if these constructors expect unusual argument counts.
94-96: Clean class type resolution
Using_isClassTypeto detectclassTypeis a clean approach. The immediate resolution usingLoader.getClassensures you handle dynamic class references gracefully.
170-170: Consistent naming
Renaming_isClassfrom the old_isClassObjectmethod is consistent. This subtle improvement helps avoid confusion.
190-192: Intuitive check for method objects
_isMethodObjectverifyingparamsis an array catches typical method param usage. If more shape checks are needed in future expansions, add them here for robust type safety.
| parseMethod(instance: any, methodName: string, methodParams: IMethodParams) { | ||
| const isMethodObject = ReflectionParser._isMethodObject(methodParams); | ||
| const params = isMethodObject ? methodParams.params : methodParams; | ||
|
|
||
| return Promise.all(params.map((param) => this.parseBasicType(param))).then((result) => { | ||
| const methodResult = instance[methodName](...result); | ||
| if (isMethodObject && methodParams.result) { | ||
| return this.parsePropsAndMethods(methodResult, methodParams.result); | ||
| } else { | ||
| return methodResult; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Modular method handling
This block properly handles both a simple param array and the new IMethod structure. Consider factoring out the isMethodObject logic into a separate helper function for clarity and to keep parseMethod minimal.
luzhuang
changed the title
* feat: support callback props in parser
engine don't support add
classand parse callback props in loader, and only support add class as parameter such as colliderShape:now i need just add
Classin engine parser, add parse some props on returned instance:onSerialize data:
Summary by CodeRabbit
New Features
Improvements