solidjs / solidjs/solid

Universal renderer: `lazy()` automatically inserts an empty text node, which may or may not be supported by the underlying renderer

Open
#2,283 5 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

documentation
Dominant language
TypeScript
Stars
36.1k
Forks
1.1k
Avg merge
9h 18m
Merged PRs (30d)
195

Description

Describe the bug

This is a regression caused by https://github.com/solidjs/solid/commit/c8fe58e9d259e463def62535f6d23454d4f30cee as far as I can tell, specifically packages/solid/src/render/component.ts line 381

return createMemo(() =>
      (Comp = comp())
        ? untrack(() => {
            if ("_SOLID_DEV_") Object.assign(Comp!, { [$DEVCOMP]: true });
            if (!ctx || sharedConfig.done) return Comp!(props);
            const c = sharedConfig.context;
            setHydrateContext(ctx);
            const r = Comp!(props);
            setHydrateContext(c);
            return r;
          })
        : ""
    ) as unknown as JSX.Element;

upon upgrading, our proprietary universal render started to fail in cases where the lazy() element was being used, because we never implemented a version of createTextNode() that would intelligently handle the empty string the second half of that ternary.

Your Example Website or App

n/a

Steps to Reproduce the Bug or Issue
  1. create a universal renderer, and implement createTextNode like so:
    createTextNode: function (_text: string): MyNodeType {
         throw new Error("Text nodes not supported!")
    },
  1. create an HTML with a lazy() component inserted.
  2. Notice your error is triggered
Expected behavior

n/a

Screenshots or Videos

n/a

Platform

n/a

Additional context

So the reason we don't support bare text nodes in our renderer is that all of our text live inside a specific base element, like so:

<label text="my actual text" />

so there was never a need to implement text nodes, and it would muddy the waters of our implementation to try. We can work around this issue just by ignoring bare text entirely. that said, if the implementation of a built in component tried to insert any other kind of text, for any reason, we'd still have this problem. One approach may just be to document this behavior, with wording along the lines of: "your renderer must at least support empty string and whitespace-only text nodes. your renderer may ignore them entirely if desired."

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start at packages/solid/src/render/component.ts line 381 and inspect how lazy() interacts with the universal renderer's createTextNode contract. Use the provided renderer reproduction to determine the supported behavior, then document or test the agreed handling; the issue does not name a test or documentation path.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
frontend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.