Fix TypeScript errors, get test suite running, and fix scene graph ordering [next] #47
Merged
Merged
Conversation
- Fix vitest SSR transform conflict (solidPlugin hot:false, process.env.NODE_ENV) - Fix camera position in test utility (defaultCamera, getBoundingClientRect) - Fix forceRefresh called synchronously in useMeasure setElement - Fix event bubbling bug in createDefaultEventRegistry (node vs intersection.object) - Fix Entity to use createMemo instead of whenMemo, preserving children across from-prop changes - Rewrite useSceneGraph to handle add/remove/reorder with R3F-style ordering: mapArray handles per-item metadata/attach/events; a createComputed(prev=>) loop manages Object3D children with correct insertion order and external-child preservation - Update tests: remove legacy Three.js r152 encoding assertions, mark unimplemented pointer capture tests as todo, fix non-stoppable onPointerEnter assertion
… loop - applySceneGraph no longer calls parent.add/remove for Object3D children - mapArray handles per-item lifecycle (metadata, attach, events) - createComputed(prev =>) loop manages Object3D add/remove/reorder: uses child.parent === parent for O(1) new-child detection, diffs prev managed set for removal, and syncs order via a dual-cursor walk over parent.children and childArray avoiding a slots[] allocation - Entity: replace whenMemo with createMemo + top-level useProps so children() is only resolved once across from-prop changes
nodeSet.values().toArray() requires Node.js 22+; CI runs Node.js 20. Array.from(nodeSet) is equivalent and universally supported. Also removes leftover debug console.log from Resource component.
Set.prototype.difference() requires Node.js 22+; CI runs Node.js 20.
commit: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR does three things:
TypeScript
src/utils.ts: FixeddefaultPropsreturn type to correctly reflect that defaults fill in optional keys. Fixedresolve()overload to cast the function case toAccessor<T>to avoid a union ambiguity. Added overloads toload()to properly distinguish single-URL vs record inputs. FixedLoadOutputto handlereadonlyarray inputs.src/hooks.ts: FixeduseLoaderto use the normalized_inputinstead of rawinputwhen writing to the registry, preventing cache misses. Fixed return types toPromiseMaybewhere appropriate.src/create-events.ts: Fixed empty object default forcreateThreeEventconfig parameter ({} as TConfig).src/components.tsx: FixedResource's<Show fallback>to cast toJSX.Element.tests/core/renderer.test.tsx: Fixed attach callback types.Test suite infrastructure
The project was migrated from Vite to tsup for building, but the test suite was never updated to match. Three things were left unported:
vitest.config.ts(new): Without this file vitest ran with defaults, which causedvite-plugin-solidto injectsolid-refreshHMR code into every module — triggering a MagicString double-edit error in Vite 5's SSR transform pipeline. Fixed by adding an explicit config withsolidPlugin({ hot: false })to disable HMR injection in the test environment, plusjsdomas the test environment with a setup file.tests/setup.ts(new): Provides aResizeObservermock for jsdom (deferred viaqueueMicrotaskto match real browser behaviour — firing synchronously triggers a "signal written in owned scope" warning). Also patchesconsole.warnto print a stack trace on that warning.src/data-structure/stack.ts: Replacedimport.meta.env?.MODEwithprocess.env.NODE_ENV.import.meta.envis Vite-specific and passes through untransformed when built with tsup/esbuild.Test fixes
src/testing/index.tsx: The test utility was passingcamera:butcreateThreeexpectsdefaultCamera:, so the camera stayed at the origin and all raycasts missed. Also overrodecanvas.getBoundingClientRectto return real dimensions — jsdom always returns zeros, causing the raycaster to computepointer = (Infinity, Infinity).src/utils/use-measure.ts:setElementnow callsforceRefresh()synchronously after setting the element. Previously bounds were only updated viaResizeObserver, which fires asynchronously (queueMicrotask), so bounds were still zero when the first event fired in tests.src/create-events.ts: Fixed a bubbling bug increateDefaultEventRegistry— thewhile (node = node.parent)walk calledgetMeta(intersection.object)on every iteration instead ofgetMeta(node), so the handler was invoked once per ancestor rather than once per intersected object.tests/core/events.test.tsx:onPointerEnter/onPointerLeaveare non-stoppable (solid-three follows DOM semantics, not r3f), so removed the incorrectstopPropagationassertion. AddedbeforeEachto reset shared mocks in the pointer capturedescribeblock to prevent cross-test contamination. Marked pointer capture tests as.todosince the feature is not yet implemented.tests/core/renderer.test.tsx: RemovedoutputEncoding/texture.encodingassertions — these APIs were removed in Three.js r152, onlyoutputColorSpace/colorSpaceremain. Updated snapshot to r164.EntitycomponentEntitywas usingwhenMemo(= createMemo(when(...))) which re-creates its reactive scope wheneverfromchanges. SinceusePropswas called inside that scope,children(() => props.children)was evaluated fresh on eachfromchange, causing JSX children to be reconstructed as new Three.js instances with new UUIDs.Fixed by calling
usePropsonce at component level and passing an accessor — the reactive scope for children is now owned by the component root and persists for its lifetime:Scene graph ordering
Previously
useSceneGraphusedmapArrayfor everything, which is keyed by item identity anddoes not respond to reordering. When a reactive list (e.g.<For>) reordered its items,Object3D.childrenstayed in the original order.We looked at prior art before settling on an approach:
insertBefore(parent, child, anchor)calls from the React reconciler and directly splicesObject3D.children— full absolute ordering including external children.parent.add(child)/parent.remove(child)and relies on implicit remounts to reorder — simple but doesn't handle reordering without teardown.__tres.objectstracking array with anchor-based insertion, but still callsparent.add()for actual attachment — the two arrays can diverge.We follow R3F's approach, adapted for Solid's reactivity model (no reconciler, so no
insertBeforeprotocol):
mapArraystill handles per-item lifecycle: solid-three metadata (parentMeta.children,childMeta.parent),attachprops, and event listener registration — with properonCleanupteardown per item.createComputed(prev =>)handles allObject3Dadd/remove/reorder. It receives the previous managedSetas its argument, diffs against the new desired array to remove stale children, inserts new children before their next already-present sibling (dispatchingadded/childaddedevents as Three.js would), and finally does a position-assignment pass to reorder: it collects the indices inparent.childrencurrently occupied by managed children, then writes the desired order into those same slots. This preserves external (non-managed) children at their positions while maintaining correct relative order among managed children.Test plan
pnpm test— 41 tests pass, 2.todo(pointer capture not yet implemented).