refactor: opt plugin system lifetime func name and notes#1303
refactor: opt plugin system lifetime func name and notes#1303
Conversation
WalkthroughRename and API changes across the plugin system: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
web-packages/test/unit/src/effects-core/composition/load-tex.spec.ts (1)
112-112: Consider addingoverridekeyword consistently.Line 51 uses the
overridekeyword, but the lifecycle hook overrides on these lines do not. For consistency and better type safety, consider addingoverrideto all lifecycle hook method declarations.Apply this pattern consistently:
-onAssetsLoadFinish (scene, options) { +override onAssetsLoadFinish (scene, options) {-onCompositionCreated (composition, scene) { +override onCompositionCreated (composition, scene) {Also applies to: 179-179, 267-267, 273-273, 322-322, 330-330
web-packages/imgui-demo/src/ge.ts (1)
19-19: Consider removing commented-out code.The commented URL is no longer in use and can be removed to keep the codebase clean.
Apply this diff:
- // const json = 'https://mdn.alipayobjects.com/mars/afts/file/A*oF1NRJG7GU4AAAAAAAAAAAAADlB4AQ'; // 春促\packages/effects-core/src/plugin-system.ts (3)
2-42: Plugin typing change looks correct; consider stricter duplicate-registration handlingSwitching to
Plugin/PluginConstructorand typingpluginsasPlugin[]is consistent with the new API; registration, sorting byorder, andunregisterPluginbehaviour are otherwise unchanged and look fine.One behavioural edge case remains: if
registerPluginis called twice with the samename, you log an error but still:
- Overwrite
pluginLoaderMap[name]with the new class, and- Push another instance into
plugins.Later,
unregisterPlugindeletespluginLoaderMap[name]and only removes the first plugin with thatname, leaving a second instance inpluginsthat will continue to receive lifecycle callbacks but is no longer inpluginLoaderMap.If duplicate registration is considered invalid (which the log message suggests), consider:
- Returning early after logging and not adding the second plugin instance, or
- Removing all plugins with that
nameinunregisterPluginto avoid stale instances.This isn’t introduced by this refactor, but it’s a good place to tighten the behaviour.
46-48:getPluginsexposes internal mutable array; consider ReadonlyArray or a copyReturning the backing
pluginsarray directly allows external callers to mutate it (e.g.,push,splice, resort), which can silently break ordering and lifecycle dispatch.If you want to keep invariants local to this module, consider:
- Changing the signature to
static getPlugins(): ReadonlyArray<Plugin>and- Either returning
pluginstyped asReadonlyArray<Plugin>orreturn plugins.slice()to hand out a shallow copy.This keeps the public API compatible from a read perspective while preventing accidental external mutation.
64-66: Clarify whetheronSceneLoadFinishshould also await plugin hooks and alignoptionssignature
onSceneLoadFinishcurrently:static onSceneLoadFinish (scene: Scene, options: SceneLoadOptions, engine: Engine) { plugins.forEach(loader => loader.onAssetsLoadFinish(scene, options, engine)); }Two points to double‑check:
- Async semantics
If anyPlugin.onAssetsLoadFinishimplementation performs async work and returns aPromise, this method will currently fire them and ignore their completion. If callers need to wait for “finish” hooks (symmetrically toonSceneLoadStart), it may be safer to aggregate:static async onSceneLoadFinish (scene: Scene, options: SceneLoadOptions, engine: Engine) { await Promise.all(plugins.map(loader => loader.onAssetsLoadFinish(scene, options, engine))); }or at least document that
onAssetsLoadFinishhooks are fire‑and‑forget and must not be awaited.
optionsoptionality consistency
onSceneLoadStarttakesoptions?: SceneLoadOptions, whereasonSceneLoadFinishrequiresoptions: SceneLoadOptions. If both are always called with options, this is fine, but if the start path allowsundefined, consider either making both optional or both required for API symmetry.Please verify intent for both points against plugin implementations and call sites.
plugin-packages/downgrade/src/downgrade-plugin.ts (1)
2-36: Align comment with actual lifecycle hook nameThe doc still says “onSceneLoadStart”, but this plugin now overrides
onAssetsLoadStart. For clarity and to avoid confusion with any PluginSystem-level naming, consider updating the comment to referenceonAssetsLoadStart(or explicitly say “scene load start phase”) to match the code.plugin-packages/alipay-downgrade/src/alipay-downgrade-plugin.ts (1)
2-35: Keep lifecycle wording consistent with method nameHere too the comment refers to “onSceneLoadStart” while the implemented hook is
onAssetsLoadStart. To reduce ambiguity for future readers, it’s worth updating the wording to match the method name (or clearly distinguish PluginSystem event vs. plugin hook if both exist).web-packages/test/unit/src/effects-core/composition/plugin.spec.ts (1)
29-41: Consider matchingonAssetsLoadFinish’s full signature for clarity
Plugin.onAssetsLoadFinishis declared as(scene, options, engine), but the test override only takes(scene, options). This works at runtime (the third arg is simply ignored), but mirroring the full signature—e.g.onAssetsLoadFinish(scene, options, _engine)—would make the intent explicit and keep the test resilient to future type changes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/effects-core/src/plugin-system.ts(2 hunks)packages/effects-core/src/plugins/plugin.ts(2 hunks)plugin-packages/alipay-downgrade/src/alipay-downgrade-plugin.ts(1 hunks)plugin-packages/downgrade/src/downgrade-plugin.ts(1 hunks)plugin-packages/model/src/plugin/model-plugin.ts(4 hunks)plugin-packages/multimedia/src/audio/audio-loader.ts(1 hunks)plugin-packages/multimedia/src/video/video-loader.ts(1 hunks)web-packages/imgui-demo/src/ge.ts(1 hunks)web-packages/test/unit/src/effects-core/composition/load-tex.spec.ts(6 hunks)web-packages/test/unit/src/effects-core/composition/plugin.spec.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin-packages/multimedia/src/video/video-loader.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-10-18T09:15:08.038Z
Learnt from: wumaolinmaoan
Repo: galacean/effects-runtime PR: 691
File: packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts:39-42
Timestamp: 2024-10-18T09:15:08.038Z
Learning: In `packages/effects-core/src/plugins/timeline/playables/color-property-mixer-playable.ts`, when accumulating color components, avoid using `clone()` methods because it can cause additional garbage collection overhead.
Applied to files:
packages/effects-core/src/plugins/plugin.tsplugin-packages/model/src/plugin/model-plugin.tspackages/effects-core/src/plugin-system.ts
📚 Learning: 2024-07-26T21:12:19.313Z
Learnt from: liuxi150
Repo: galacean/effects-runtime PR: 0
File: :0-0
Timestamp: 2024-07-26T21:12:19.313Z
Learning: Imports in `plugin-packages/model/src/utility/plugin-helper.ts` do not need to be merged as per project preference or coding standards.
Applied to files:
plugin-packages/model/src/plugin/model-plugin.tsplugin-packages/downgrade/src/downgrade-plugin.tsweb-packages/test/unit/src/effects-core/composition/plugin.spec.tspackages/effects-core/src/plugin-system.ts
🧬 Code graph analysis (4)
plugin-packages/alipay-downgrade/src/alipay-downgrade-plugin.ts (1)
packages/effects-core/src/scene.ts (2)
Scene(12-35)SceneLoadOptions(62-140)
web-packages/test/unit/src/effects-core/composition/load-tex.spec.ts (3)
packages/effects-core/src/plugin-system.ts (1)
registerPlugin(18-31)packages/effects-core/src/scene.ts (1)
Scene(12-35)packages/effects-core/src/gl/index.ts (1)
glContext(8-8)
plugin-packages/downgrade/src/downgrade-plugin.ts (1)
packages/effects-core/src/scene.ts (2)
Scene(12-35)SceneLoadOptions(62-140)
web-packages/test/unit/src/effects-core/composition/plugin.spec.ts (2)
packages/effects-core/src/vfx-item.ts (2)
composition(184-186)composition(191-199)packages/effects-core/src/composition.ts (1)
Composition(132-990)
🔇 Additional comments (9)
web-packages/test/unit/src/effects-core/composition/load-tex.spec.ts (3)
3-3: LGTM! Import updated correctly.The import statement has been properly updated to use
Plugininstead ofAbstractPlugin, aligning with the plugin system refactoring.
50-61: LGTM! Test plugin updated correctly.The test plugin has been properly refactored to extend
Pluginand use theonAssetsLoadFinishlifecycle hook. Theoverridekeyword is correctly used for type safety.
111-124: LGTM! All test plugins updated consistently.All test plugins have been correctly refactored to:
- Extend
Plugininstead ofAbstractPlugin- Use
onAssetsLoadFinishinstead ofprepareResource- Use
onCompositionCreatedinstead ofonCompositionConstructedThe test logic remains unchanged, ensuring the refactoring maintains existing test coverage and behavior.
Also applies to: 178-193, 266-277, 321-334
web-packages/imgui-demo/src/ge.ts (1)
25-25: The URL is accessible and returns valid content. This appears to be an intentional demo asset swap (the previous URL is commented on line 19), appropriate for a demo/test file. No concerns.packages/effects-core/src/plugin-system.ts (2)
50-56: Composition lifecycle rename/dispatch looks consistentThe renaming to
onCompositionCreated/onCompositionDestroyand the corresponding dispatch ininitializeComposition/destroyCompositionis straightforward and matches the new Plugin lifecycle naming. No issues from a control-flow or typing standpoint.
58-62:onSceneLoadStartasync aggregation is soundUsing
async onSceneLoadStartwithreturn Promise.all(plugins.map(plugin => plugin.onAssetsLoadStart(...)))correctly aggregates all pluginonAssetsLoadStarthooks, propagating both rejections and synchronous throws through the returned promise. This keeps the call site able to await all start hooks. Looks good.packages/effects-core/src/plugins/plugin.ts (1)
5-47: Core Plugin API migration looks consistentThe renaming from
AbstractPlugintoPluginplus the new lifecycle hooks (onAssetsLoadStart,onAssetsLoadFinish,onCompositionCreated,onCompositionDestroy) is internally consistent and matches the usage in the updated plugins and tests. ThePluginConstructortype and the defaultname = 'Plugin'also look reasonable; just keep in mind that logs or tooling relying on an empty default name will now see'Plugin'when subclasses don’t override it.plugin-packages/multimedia/src/audio/audio-loader.ts (1)
2-22: AudioLoader migration toPluginandonAssetsLoadStartlooks goodThe switch to
Pluginand renamingprocessAssets→onAssetsLoadStartkeeps the original loading logic intact and aligns with the new lifecycle API. No issues from a functional standpoint.plugin-packages/model/src/plugin/model-plugin.ts (1)
17-100: ModelPlugin lifecycle rename and base-class switch look correctThe move to
Pluginplus the renames toonAssetsLoadStart,onAssetsLoadFinish, andonCompositionCreatedkeep the original 3D-model behavior intact (static resource preload, scene preprocessing, shader setup, and injection ofModelPluginComponent). The overrides and their parameter types line up with the newPluginAPI.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugin-packages/alipay-downgrade/src/alipay-downgrade-plugin.ts (1)
14-35: Consider extracting common downgrade logic.
AlipayDowngradePluginandDowngradePluginshare nearly identical implementations. The only difference is the class name. Consider extracting the common downgrade checking and renderLevel assignment logic into a shared base class or utility function to reduce duplication.Example approach:
// Create a shared base class abstract class BaseDowngradePlugin extends Plugin { constructor() { super(); this.order = 1; } override async onAssetsLoadStart(scene: Scene, options?: SceneLoadOptions): Promise<void> { // Existing logic here } } // Then extend it export class AlipayDowngradePlugin extends BaseDowngradePlugin {} export class DowngradePlugin extends BaseDowngradePlugin {}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/effects-core/src/asset-manager.ts(2 hunks)packages/effects-core/src/plugin-system.ts(2 hunks)packages/effects-core/src/scene-loader.ts(1 hunks)packages/effects-threejs/src/three-display-object.ts(1 hunks)plugin-packages/alipay-downgrade/src/alipay-downgrade-plugin.ts(1 hunks)plugin-packages/alipay-downgrade/test/src/player.spec.ts(2 hunks)plugin-packages/downgrade/src/downgrade-plugin.ts(1 hunks)plugin-packages/downgrade/test/src/player.spec.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- plugin-packages/alipay-downgrade/test/src/player.spec.ts
- packages/effects-core/src/scene-loader.ts
- packages/effects-threejs/src/three-display-object.ts
- plugin-packages/downgrade/test/src/player.spec.ts
- packages/effects-core/src/asset-manager.ts
- packages/effects-core/src/plugin-system.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-07-26T21:12:19.313Z
Learnt from: liuxi150
Repo: galacean/effects-runtime PR: 0
File: :0-0
Timestamp: 2024-07-26T21:12:19.313Z
Learning: Imports in `plugin-packages/model/src/utility/plugin-helper.ts` do not need to be merged as per project preference or coding standards.
Applied to files:
plugin-packages/downgrade/src/downgrade-plugin.tsplugin-packages/alipay-downgrade/src/alipay-downgrade-plugin.ts
🧬 Code graph analysis (1)
plugin-packages/alipay-downgrade/src/alipay-downgrade-plugin.ts (1)
packages/effects-core/src/scene.ts (2)
Scene(12-35)SceneLoadOptions(62-140)
🔇 Additional comments (4)
plugin-packages/downgrade/src/downgrade-plugin.ts (3)
2-2: LGTM: Base class import updated.The import change from
AbstractPlugintoPluginaligns with the plugin system refactoring.
16-19: LGTM: Explicit constructor with execution order.The constructor explicitly sets the execution order, making it clear this plugin runs early in the lifecycle.
9-9: LGTM: Lifecycle hook renamed with updated documentation.The rename from
processAssetstoonAssetsLoadStartis more descriptive and aligns with the new plugin lifecycle naming. The comment has been updated accordingly.Also applies to: 21-21
plugin-packages/alipay-downgrade/src/alipay-downgrade-plugin.ts (1)
2-2: LGTM: Plugin system refactoring applied consistently.The changes mirror those in
downgrade-plugin.ts: base class updated toPlugin, explicit constructor added, and lifecycle hook renamed toonAssetsLoadStart.Also applies to: 14-18, 20-20
2 participants
Summary by CodeRabbit
Refactor
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.