Skip to content

Feat(solid): Handle anchor node and add Dynamic and Portal#169

Merged
kommander merged 16 commits intomainfrom
anchor-node-test
Sep 18, 2025
Merged

Feat(solid): Handle anchor node and add Dynamic and Portal#169
kommander merged 16 commits intomainfrom
anchor-node-test

Conversation

@Adictya
Copy link
Collaborator

@Adictya Adictya commented Sep 17, 2025

  • Simple bun test should also work now
  • Added tests for Dynamic and Portal
  • Brought in universal renderer for custom addAnchorNode logic
  • Fixes a bunch of bugs around Show and For
  • We throw on not adding unwrapped text/span.

@Adictya
Copy link
Collaborator Author

Adictya commented Sep 17, 2025

WIP, fixing and linting
Done

@Adictya Adictya changed the title Handle anchor node and add Dynamic and Portal WIP: Handle anchor node and add Dynamic and Portal Sep 17, 2025
@Adictya Adictya changed the title WIP: Handle anchor node and add Dynamic and Portal Feat(solid): Handle anchor node and add Dynamic and Portal Sep 17, 2025

children = testSetup.renderer.root.getChildren()[0]!.getChildren()
expect(children.length).toBe(1)
expect(children.length).toBe(2)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Solid subs existing element with the empty node so number of children remain same

@Adictya Adictya changed the title Feat(solid): Handle anchor node and add Dynamic and Portal WIP: Feat(solid): Handle anchor node and add Dynamic and Portal Sep 17, 2025
@Adictya Adictya changed the title WIP: Feat(solid): Handle anchor node and add Dynamic and Portal Feat(solid): Handle anchor node and add Dynamic and Portal Sep 17, 2025
@@ -0,0 +1,130 @@
import { BaseRenderable, isTextNodeRenderable, TextNodeRenderable, TextRenderable } from ".."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are the slots something reconciler specific that we could have in the solid package? For core itself they don't seem to have a use-case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah will have to explore around the api to make it work well for core. But I was thinking if it had a replaceSelf method, which allows us to create a renderable with slots and expose those such that on usage we could just slot in custom renderables. For eg Dialog component with header, content and footer slots.

This is sort of a low cost node that can be replaced when needed.

I will move it for now tho.

await testSetup.renderOnce()

setComponentType("box")
await testSetup.renderOnce()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this missing an expectation or just testing that it doesn't crash?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup I'll document


it("should throw on rendering text without parent <text> element", async () => {
expect(
testRender(() => <box>This text is not wrapped in a text element</box>, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I hope we can get this to work in the future, would be so neat.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Already on it dw

kommander
kommander previously approved these changes Sep 18, 2025
Copy link
Collaborator

@kommander kommander left a comment

Choose a reason for hiding this comment

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

I would move the Slot renderables to the solid package, as they are reconciler specific, other than that looks great 👍

@Adictya Adictya requested a review from kommander September 18, 2025 13:53
@kommander kommander merged commit 7ff2578 into main Sep 18, 2025
3 checks passed
@Adictya Adictya deleted the anchor-node-test branch September 18, 2025 18:30
msmps pushed a commit that referenced this pull request Jan 8, 2026
- Simple `bun test` should also work now
- Added tests for Dynamic and Portal
- Brought in universal renderer for custom addAnchorNode logic
- Fixes a bunch of bugs around Show and For 
- We throw on not adding unwrapped text/span.
Hona added a commit that referenced this pull request Feb 11, 2026
…eplacement

The forked universal renderer diverged from solid-js's upstream behavior
in cleanChildren(). When called without a marker or replacement (e.g.
when Show/Switch hides content), it returned `replacement` which is
`undefined`, instead of returning "" like solid-js does.

This caused undefined to propagate through the reactive graph as the
`current` value in insertExpression, eventually corrupting downstream
memo reads.

Introduced in #169 when the universal renderer was forked.
Adictya pushed a commit that referenced this pull request Feb 11, 2026
…eplacement (#670)

## Summary

- Fix `cleanChildren` in the forked universal renderer to return `""`
instead of `undefined` when called without a replacement, matching
solid-js's upstream behavior

## Problem

When `cleanChildren` is called without a `marker` or `replacement` (e.g.
when `Show`/`Switch` hides content), the no-marker early-return path
returns `replacement` — which is `undefined`. The solid-js upstream
returns `""`.

This `undefined` propagates as the `current` value through
`insertExpression`'s reactive chain, eventually corrupting downstream
memo reads and causing crashes like:

```
TypeError: undefined is not an object (evaluating 'grouped().length')
```

Introduced in #169 when the universal renderer was forked from solid-js.

## Fix

```diff
- return replacement
+ return replacement ?? ""
```
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