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

refactor(components): enhance component accessor type safety with generics #18282

Merged
merged 1 commit into from
Feb 13, 2025

Conversation

9romise
Copy link
Contributor

@9romise 9romise commented Feb 6, 2025

Re: #

Changelog

  • Convert component accessor methods to use generic types for precise type inference
  • Improve developer experience with auto-completion and type checking

Before:

const clz = node.getComponent('clz') as CLZ | null

After:

const clz = node.getComponent<CLZ>('clz')

Continuous Integration

This pull request:

  • needs automatic test cases check.

    Manual trigger with @cocos-robot run test cases afterward.

  • does not change any runtime related code or build configuration

    If any reviewer thinks the CI checks are needed, please uncheck this option, then close and reopen the issue.


Compatibility Check

This pull request:

  • changes public API, and have ensured backward compatibility with deprecated features.
  • affects platform compatibility, e.g. system version, browser version, platform sdk version, platform toolchain, language version, hardware compatibility etc.
  • affects file structure of the build package or build configuration which requires user project upgrade.
  • introduces breaking changes, please list all changes, affected features and the scope of violation.

@dumganhar
Copy link
Contributor

@cocos-robot run test cases

Copy link

github-actions bot commented Feb 7, 2025

@9romise, Please check the result of run test cases:

Task Details

Platform build boot runned crashScene FailScene
web-mobile PASS PASS FAIL
ios PASS PASS FAIL reflection-probe
mac PASS PASS FAIL reflection-probe

Copy link

github-actions bot commented Feb 7, 2025

@9romise, Please check the result of run test cases:

Task Details

@minggo
Copy link
Contributor

minggo commented Feb 13, 2025

@cocos-robot run test cases

Copy link

@9romise, Please check the result of run test cases:

Task Details

Platform build boot runned crashScene FailScene
web-mobile PASS PASS FAIL SkewTest
ios PASS PASS FAIL reflection-probe
mac PASS PASS FAIL reflection-probe

Copy link

@9romise, Please check the result of run test cases:

Task Details

Platform build boot runned crashScene FailScene
windows PASS PASS PASS
android PASS PASS PASS
wechatgame PASS PASS FAIL SkewTest

@minggo minggo merged commit 4c693d2 into cocos:v3.8.6 Feb 13, 2025
13 checks passed
@9romise 9romise deleted the type/component branch February 13, 2025 05:39
@dumganhar
Copy link
Contributor

dumganhar commented Feb 18, 2025

Hi, @9romise , @minggo

This code will break the compatibilty in engine, for example, In spine/skeleton.ts.

debugDraw = debugDrawNode.addComponent('cc.Graphics') as Graphics;

ESLint report: This assertion is unnecessary since it does not change the type of the expression.

If I save the file, eslint configuration will automatically remove as Graphics, so the code will be

debugDraw = debugDrawNode.addComponent('cc.Graphics');

But it causes another TS compiler error:

Type 'Component' is missing the following properties from type 'Graphics': lineWidth, lineJoin, lineCap, strokeColor, and 101 more.

Yes, we could modify the code to

debugDraw = debugDrawNode.addComponent<Graphics>('cc.Graphics');

to resolve the issue in engine. But if our users also write the same kinds of code, they will complain about the bad compatibilty.

And I think the two kinds of addComponent declaration doesn't have much differences.

debugDraw = debugDrawNode.addComponent('cc.Graphics'); // Old way and new way both return `Component | null` type
debugDraw = debugDrawNode.addComponent<Graphics>('cc.Graphics'); // New way in this PR, Return Graphics type
debugDraw = debugDrawNode.addComponent('cc.Graphics') as Graphics; // Old way, Return Graphics type

I suggest to revert this PR for better compatibility. What do you think?

@minggo
Copy link
Contributor

minggo commented Feb 18, 2025

@dumganhar can we keep both of the two usage?

@dumganhar
Copy link
Contributor

@minggo It seems we could add the new overrode addComponent for the compatibilty, let me investigate more.

dumganhar added a commit to dumganhar/cocos-engine that referenced this pull request Feb 18, 2025
@dumganhar
Copy link
Contributor

Sent a PR for this:

#18345

@minggo, @9romise , please review it.

@9romise 9romise restored the type/component branch February 18, 2025 03:30
@9romise 9romise deleted the type/component branch February 18, 2025 03:31
minggo pushed a commit that referenced this pull request Feb 18, 2025
bofeng-song pushed a commit to bofeng-song/engine that referenced this pull request Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants