Skip to content

Support editing a property of type array without oneOf like animation startEvents#1572

Closed
vincentfretin wants to merge 1 commit into
mainfrom
fix-type-array-of-string
Closed

Support editing a property of type array without oneOf like animation startEvents#1572
vincentfretin wants to merge 1 commit into
mainfrom
fix-type-array-of-string

Conversation

@vincentfretin

Copy link
Copy Markdown
Collaborator

For array-type properties without oneOf (rendered via InputWidget), stringify the array for display using schema.stringify (", "-joined) and parse the comma-separated string back to an array in onChange. This ensures A-Frame always receives an array for array-type properties — avoiding arrayStringify being called on a raw string downstream (which would crash with "t.join is not a function").

Backport of aframevr/aframe-inspector#855

… startEvents

For array-type properties without oneOf (rendered via InputWidget),
stringify the array for display using schema.stringify (", "-joined)
and parse the comma-separated string back to an array in onChange.
This ensures A-Frame always receives an array for array-type properties
— avoiding arrayStringify being called on a raw string downstream
(which would crash with "t.join is not a function").

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@vincentfretin
vincentfretin requested a review from kfarr as a code owner April 23, 2026 09:49
@claude

claude Bot commented Apr 23, 2026

Copy link
Copy Markdown

Code Review: Support editing array-type properties without oneOf (e.g. animation startEvents)

Summary: This is a clean backport of aframe-inspector#855 that fixes a crash when editing array-type A-Frame component properties that don't have a oneOf schema (like animation.startEvents). Without this fix, EntityUpdateCommand calls schemaProperty.stringify(value).join() on a raw string (instead of an array), causing "t.join is not a function".


What the fix does

In PropertyRow.jsx, introduces a isFreeInputArray flag:

  • Display side: converts the raw array to a comma-separated string via schema.stringify() so InputWidget receives a string as expected
  • onChange side: parses the string back to an array via schema.parse() before dispatching to EntityUpdateCommand, so the downstream .join() call always gets an array

This correctly distinguishes from the oneOf case, where SelectWidget needs the raw array.


Code Quality

  • The fix is minimal, focused, and well-commented — exactly what a backport should be.
  • The isFreeInputArray naming is clear and intention-revealing.
  • The else if pattern mirrors the existing string type guard just above it (line 75–77), keeping the code consistent.
  • Comments explain the why, not just the what — specifically, why the oneOf case is excluded.

Potential Issues / Questions

Minor: Redundant null guard!(props.schema.oneOf && props.schema.oneOf.length > 0) is the same pattern used at line 137, so it's consistent. Could be simplified to !props.schema.oneOf?.length, but no behavior difference.

Edge case: schema.parse() on invalid input — If a user types "a, b, c" into a property expecting numbers, schema.parse() may produce unexpected values or silent coercions at the A-Frame component level. This was also true before (entering invalid values in InputWidget has always been possible), so this is not a regression.

typeof value === 'string' in onChange guard — This will always be true coming from InputWidget (which emits strings), so the guard is correct and safe, though it's slightly defensive. Not an issue.

Non-array initial value — The Array.isArray(value) check on the display side means if value is somehow already a string when isFreeInputArray is true (e.g., freshly loaded from DOM before A-Frame parses it), it won't be double-stringified. Good defensive check.


Test Coverage

No tests exist for PropertyRow (consistent with the rest of the React editor in this repo). The upstream PR this backports from has its own test coverage. Given this is a targeted bug fix mirroring upstream, this is acceptable — but a future investment in React component testing would help catch regressions earlier.


Verdict

Approve. This is a correct, minimal fix for a real crash bug. The logic is sound, the code is clean, and it faithfully follows the upstream fix. No blocking concerns.

@vincentfretin

Copy link
Copy Markdown
Collaborator Author

Oh you already did a similar fix in
2162f9e actually, interesting, closing for now.
Your change doesn't trim after the split(','), so it may or may not have an issue with "one, two, three".

@kfarr

kfarr commented Apr 23, 2026

Copy link
Copy Markdown
Collaborator

thanks yeah this is one of many bugs I found and solved and forgot about

@vincentfretin

Copy link
Copy Markdown
Collaborator Author

I modified it with extra goodies in #1573 to backport some changes I did in aframe-inspector/aframe-editor you may benefit in future features.

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.

2 participants