Inner render function triggers solid/components-return-once

[tronikelis]

Describe the bug
Inner render functions trigger the early return rule, but they shouldn't

To Reproduce
https://playground.solidjs.com/anonymous/7d1abcec-af91-4975-8058-dfd1983f2ff7

function Counter(props: ComponentProps<"button">) {
  const [c, setC] = createSignal(0);

  setInterval(() => setC((c) => c + 1), 500);

  const renderContent = () => {
    if (c() < 10) {
      return (
        <div>
          <p>{10}</p>
        </div>
      );
    }

    if (c() >= 10 && c() < 20) {
      return <p>20</p>;
    }

    return <p>out of bounds</p>;
  };

  return <div>{renderContent()}</div>;
}

Expected behavior
As I understand this is supported in solid as renderContent is within an effect inside the JSX which reacts to changes, so this shouldnt trigger this rule solid/components-return-once

Environment (please complete the following information):

• OS: Windows 10
• Node version 20.10.0
• eslint-plugin-solid version 0.13.0
• eslint version 8.56.0

• I would be willing to contribute a PR to fix this issue

[tronikelis]

So I tried fixing this

https://github.com/Tronikelis/eslint-plugin-solid/blob/1d3b1437b50627bcbdb8879a8074be4ec5366a37/src/rules/components-return-once.ts

I removed the autofix logic though, passes all old tests and the new ones

Now I rewrote the rule because the current rule too complex for me (skill issue?) so even if you dont want to rewrite it maybe you could get some ideas about how to do it, or you could accept the PR with that change

[joshwilsonvu]

Thanks for trying to fix this! I know some of these rules can be hard to parse—in this case a lot of the complexity is from efficiently tracking which functions have JSX, making them candidates for being Solid components.

You're right, a render function will be re-run in an effect, so it doesn't have the same problem that early returns do for a top-level component: never, ever running one branch or the other.

There was already an attempt to catch render functions here:

  const onFunctionExit = (node: FunctionNode) => {
      if (
        (node.type === "FunctionDeclaration" && node.id?.name?.match(/^[a-z]/)) ||
        // "render props" aren't components

The problem is that this only checks for FunctionDeclarations (like function foo() { ... }); it misses arrow functions like in your example. If you change renderContent into a function declaration, you'll see that the rule stops warning. So, I think the fix should be making this check more inclusive and checking all inline functions, no matter what syntax they use. Somehow I don't already have a utility function to do this, so I'll handle making one and fixing this check.

If you're curious why we're just checking the name of the function, it's because your renderContent could just as easily be rendered as a component, like <RenderContent>, and then it would only run once. It's really hard to statically analyze how a given function might be used, so we just rely on the lowercase/uppercase naming convention.

---

FYI: Though it's not the same problem, a render function with early returns isn't optimal. In Solid, anywhere you run JSX like <p>20</p> immediately creates DOM nodes (instead of cheap plain objects like in React). So code like this will end up creating and destroying DOM nodes more often than necessary. Using <Show> or <Switch> and <Match> instead is more efficient.

Currently, this rule isn't set up to warn about that issue, but maybe it should, at least with a different message.

[tronikelis]

Thanks for such a verbose answer 👍

instead of cheap plain objects like in React

Hmm I wonder about this actually, from my experience using react the performance problem was the diffing and a lot of GC, so it would be interesting to benchmark does it have a large impact with an early return in an inner render function, I use them in a semi large solid app and I haven't noticed anything, because solidjs is just so efficient without having to do anything

[tronikelis]

So, I think the fix should be making this check more inclusive and checking all inline functions,

We need to include this edge case as well

function Component()

	// ifs not fine
	const renderContent = () => {
		// ifs are fine
		const renderContentInner = () => {
			// ifs are fine
			// ifs are fine no matter what nesting level this is
		}
	
		return <>{renderContentInner()}</>
	}

return <></>

I solved this just by going to the outer function until the next outer function is not a component, but this implementation is probably very slow

[joshwilsonvu]

Screen.Recording.2023-12-30.at.3.07.21.PM.mov

Yes, SolidJS is efficient by default when you write idiomatic code, but in the devtools you can see that the DOM nodes are indeed getting fully replaced instead of updated each time renderContent reruns. You probably aren't noticing it since it's just a few nodes, but this is more expensive than React rerenders. I've heard it described as something like

React is slowish by default, and can be optimized to become faster; Solid is fast by default but can become very, very slow when used incorrectly.

Just something to be aware of!

---

Added that as a test case, all's working now.

[tronikelis]

thanks

[tronikelis]

You probably aren't noticing it since it's just a few nodes

Yeah, I am using them in place of deeply nested jsx so its only a few divs that are getting recreated, but the code clarity is much much better with this tradeoff

But I'll keep this in mind, thanks