Closed Tronikelis closed 6 months ago
So I tried fixing this
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
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 FunctionDeclaration
s (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.
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
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
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.
thanks
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
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
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 rulesolid/components-return-once
Environment (please complete the following information):
eslint-plugin-solid
version 0.13.0eslint
version 8.56.0