Skip to content

refactor(components): eliminate ComponentData classes and engine schema usage#8876

Merged
willeastcott merged 4 commits into
mainfrom
refactor/eliminate-component-data
Jun 11, 2026
Merged

refactor(components): eliminate ComponentData classes and engine schema usage#8876
willeastcott merged 4 commits into
mainfrom
refactor/eliminate-component-data

Conversation

@willeastcott

@willeastcott willeastcott commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Description

Completes the multi-PR effort to eliminate the legacy ComponentData classes (latest prior step: #8873). After that PR, all 23 *ComponentData classes defined only enabled = true, so the schema/buildAccessors machinery was generating exactly one accessor per component — enabled, backed by the store data object.

What changed

  • Base Component now has a _enabled = true field and a real enabled getter/setter that fires the same 'set' event the generated accessor did (the unconditional fire is load-bearing for script and zone, which listen for set_enabled even when the value is unchanged).
  • Base ComponentSystem: DataType is now optional in addComponent (an empty object is stored when absent), and initializeComponentData's properties parameter is now optional — the descriptor loop is the legacy path for external schema-based components, and when it is omitted the base initializer applies the enabled state from the data block directly. Engine systems just call super.initializeComponentData(component, data); element, sprite and the layout systems, which deliberately initialize enabled before their asset-loading setters, opt out by passing an explicit empty list.
  • All 23 systems: removed _schema, this.schema, this.DataType, the data.js imports and the module-level Component._buildAccessors(...) calls; deleted all 23 data.js files. They were never exported from index.js, so there is no public API break.
  • Targeted fixes for paths the migration would otherwise have silently broken:
    • The animation and sprite system update loops read enabled off the (now-empty) store record — rewritten to read the component.
    • JointComponent.initFromData raw-wrote this._enabled, which would clobber the new backing field and suppress the onDisable transition (leaking the Ammo constraint when a joint is added disabled) — enabled is removed from its property list.
    • audio-listener and joint were the only systems relying on the default cloneComponent (which clones from store data); both get explicit overrides.
    • Removed the now-redundant custom enabled accessors from ElementComponent and ScriptComponent (the base accessor is identical to ScriptComponent's old one).

playcanvas-spine compatibility

The legacy machinery (buildAccessors/_buildAccessors, the data getter, the constructor schema fallback, store data records, schema-driven init, default cloneComponent) is kept in the base classes and annotated as a legacy path. playcanvas-spine defines a 9-property schema + DataType and relies on all of it via the base constructor fallback. Since no engine component exercises that path anymore, a new test (test/framework/components/legacy-component.test.mjs) locks it down: per-instance accessor building, data-backed enabled, set_* events, and default cloning from live store data. Once spine is modernized, a follow-up PR can delete the whole mechanism.

Behavior notes

  • _enabled is now an own enumerable property on every component (previously the value lived in the store and the accessors were non-enumerable).
  • component.data returns {} for engine components; external code that raw-wrote component.data.enabled (already documented as not firing events) now has no effect.
  • Reading component.enabled inside a subclass constructor previously threw (no store record yet); it now returns true.

Verification

  • npm test — 1,814 passing, including the 5 new legacy-path tests and the "clones the enabled state" tests for all 23 component types
  • npm run lint, npm run build:types, npm run test:types — clean
  • Spine smoke test in the examples browser (spineboy example with the bundled playcanvas-spine.3.8.js): accessors built via the constructor fallback, portal animation renders and advances, entity.spine.enabled toggling round-trips through the data store, and entity.clone() produces a working second spine instance

Checklist

  • I have read the contributing guidelines
  • My code follows the project's coding standards
  • This PR focuses on a single change

🤖 Generated with Claude Code

…ma usage

All 23 ComponentData classes defined only `enabled = true` after the
multi-PR effort to move component properties onto the components
themselves (latest: #8873). This completes the elimination:

- Base Component now has a field-backed `enabled` getter/setter that
  fires the same 'set' event the schema-generated accessor did
- All engine-internal _schema / this.schema / this.DataType /
  Component._buildAccessors usage is removed and the 23 data.js files
  are deleted (they were not exported from index.js)
- The legacy machinery (buildAccessors, the `data` getter, the
  constructor schema fallback, store data records, schema-driven init)
  is kept in the base classes for external legacy-style components
  such as playcanvas-spine, and is now covered by a dedicated test

Targeted fixes for paths the migration would have silently broken:

- animation/sprite system update loops read enabled from the store
  record; they now read the component
- JointComponent.initFromData raw-wrote this._enabled, which would
  clobber the new backing field and suppress the onDisable transition
- audio-listener and joint systems relied on the default cloneComponent
  (which clones from store data); both get explicit overrides
- Removed the now-redundant custom enabled accessors from
  ElementComponent and ScriptComponent

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jun 11, 2026

Copy link
Copy Markdown

Public API report

This PR changes the public API surface (+20 / −20), per the docs' rules (@ignore / @Private / undocumented are excluded).

Show API diff
-AnimComponent.set enabled(arg: boolean)
+AnimComponent.set enabled(value: boolean)
-AnimationComponent.set enabled(arg: boolean)
+AnimationComponent.set enabled(value: boolean)
-AudioListenerComponent.set enabled(arg: boolean)
+AudioListenerComponent.set enabled(value: boolean)
-ButtonComponent.set enabled(arg: boolean)
+ButtonComponent.set enabled(value: boolean)
-CameraComponent.set enabled(arg: boolean)
+CameraComponent.set enabled(value: boolean)
-CollisionComponent.set enabled(arg: boolean)
+CollisionComponent.set enabled(value: boolean)
-Component.set enabled(arg: boolean)
+Component.set enabled(value: boolean)
-GSplatComponent.set enabled(arg: boolean)
+GSplatComponent.set enabled(value: boolean)
-LayoutChildComponent.set enabled(arg: boolean)
+LayoutChildComponent.set enabled(value: boolean)
-LayoutGroupComponent.set enabled(arg: boolean)
+LayoutGroupComponent.set enabled(value: boolean)
-LightComponent.set enabled(arg: boolean)
+LightComponent.set enabled(value: boolean)
-ModelComponent.set enabled(arg: boolean)
+ModelComponent.set enabled(value: boolean)
-ParticleSystemComponent.set enabled(arg: boolean)
+ParticleSystemComponent.set enabled(value: boolean)
-RenderComponent.set enabled(arg: boolean)
+RenderComponent.set enabled(value: boolean)
-RigidBodyComponent.set enabled(arg: boolean)
+RigidBodyComponent.set enabled(value: boolean)
-ScreenComponent.set enabled(arg: boolean)
+ScreenComponent.set enabled(value: boolean)
-ScrollViewComponent.set enabled(arg: boolean)
+ScrollViewComponent.set enabled(value: boolean)
-ScrollbarComponent.set enabled(arg: boolean)
+ScrollbarComponent.set enabled(value: boolean)
-SoundComponent.set enabled(arg: boolean)
+SoundComponent.set enabled(value: boolean)
-SpriteComponent.set enabled(arg: boolean)
+SpriteComponent.set enabled(value: boolean)

Informational only — this never fails the build.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR completes the engine-side migration away from legacy ComponentData / schema-driven accessors by making enabled a real Component field and removing the now-redundant data.js / schema plumbing from engine component systems, while explicitly preserving (and testing) the legacy schema+DataType path for external plugins.

Changes:

  • Added a real Component.enabled getter/setter backed by _enabled, while keeping the legacy schema-based accessor path functional.
  • Updated ComponentSystem to tolerate systems without DataType and to apply legacy data defaults only when the property exists on the data object.
  • Removed schema/DataType/data.js usage from engine systems shown here, and added a dedicated unit test locking down the legacy schema-based component behavior.

Reviewed changes

Copilot reviewed 52 out of 52 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test/framework/components/legacy-component.test.mjs Adds coverage for the preserved legacy schema/DataType path (per-instance accessors, data-backed enabled, set events, default clone).
src/framework/components/component.js Introduces _enabled backing field and real enabled accessor; documents and preserves legacy accessor building and data semantics.
src/framework/components/system.js Makes DataType optional in addComponent; narrows legacy-default application; preserves onEnable triggering post-init.
src/framework/components/particle-system/system.js Ensures initialization respects enabled without early enable/disable transitions by pre-seeding _enabled before other setters.
src/framework/components/animation/system.js Updates update-loop enabled checks to read from the component (not store data).
src/framework/components/sprite/system.js Updates update-loop enabled checks to read from the component (not store data).
src/framework/components/joint/system.js Removes schema/DataType and adds explicit cloneComponent to preserve enabled cloning behavior.
src/framework/components/audio-listener/system.js Removes schema/DataType and adds explicit cloneComponent to preserve enabled cloning behavior.
src/framework/components/zone/system.js Removes schema/DataType and relies on direct enabled initialization.
src/framework/components/script/component.js Removes redundant enabled override now that base Component provides the same behavior.
src/framework/components/element/component.js Removes redundant enabled accessor now that base Component provides it.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@willeastcott willeastcott requested a review from kpal81xd June 11, 2026 17:49
@LeXXik

LeXXik commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Just a note from my experience creating custom components for the engine. I found component schemas useful, as it was able to catch malformed options early. For example:

entity.addComponent('component', {
    optionA: true,
    opionB: true
});

Internally the engine would initialize component data using the provided options object. I would then verify each property of the object against the schema and issue a warning, if the property is not found in the schema (e.g. opionB typo). Perhaps, we could have something similar in debug-only builds.

…izeComponentData

Passing ['enabled'] to super.initializeComponentData kept every engine
system speaking the schema dialect this PR eliminates. The base
initializer now applies the enabled state from the data block directly
when no properties list is given, so engine systems simply call
super.initializeComponentData(component, data).

- The properties param is now optional and documented as the legacy
  path for external schema-based components (e.g. playcanvas-spine)
- addComponent no longer passes an empty properties array
- element, sprite, layout-child and layout-group initialize enabled
  themselves (deliberately before asset-loading setters), so they opt
  out of the base handling by passing an explicit empty list
- audio-listener's initializeComponentData override only re-routed the
  enabled property and is removed entirely

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@willeastcott

Copy link
Copy Markdown
Contributor Author

Thanks for your thoughts on this, @LeXXik. We can probably address this in a follow up PR. 🤔

@willeastcott willeastcott merged commit 321fd9d into main Jun 11, 2026
9 checks passed
@willeastcott willeastcott deleted the refactor/eliminate-component-data branch June 11, 2026 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Request for a new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants