Preventing infinite loops #1338
Conversation
🦋 Changeset detectedLatest commit: 407a1c8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Try the Instant Preview in Online PlaygroundInstall the Instant Preview to Your LocalPublished Instant Preview Packages:
|
marekdedic
force-pushed
the
consistent-selector-style-infinite-loop-fix
branch
2 times, most recently
from
563a4c8 to
b05ce6b
Compare
marekdedic
force-pushed
the
consistent-selector-style-infinite-loop-fix
branch
3 times, most recently
from
9c24dd3 to
82b3e76
Compare
| ) => T | null; | ||
| }; | ||
|
|
||
| export function ASTSearchHelper<T>(startNode: ASTNode, nodeResolvers: NodeResolvers<T>): T | null { |
There was a problem hiding this comment.
What is the use of this function? I don't understand it. Can you explain?
There was a problem hiding this comment.
svelte/no-dynamic-slot-name now causes an error.
It worked fine before.
There was a problem hiding this comment.
This function is the core of this PR. It abstracts away a common pattern where you search through the AST and depending on the node you return some value or look in another node (usually parent, child, but also e.g. variable definition).
As we noticed in the linked PR, there is an issue in both consistent-selector-style as well as no-navigation-without-resolve where you could get an infinite recursion in the rule code.
This function prevents infinite loops as well. The pattern seemed to repeat in multiple places in the codebase, so I created this abstraction.
There was a problem hiding this comment.
Re: no-dynamic-slot-name - Ugh, that's not in the tests, could you add it? The playground doesn't work for me.
Since the rule is deprecated, my first choice would be to leave it as it is and remove the change to this rule, what do you think?
There was a problem hiding this comment.
This function prevents infinite loops as well. The pattern seemed to repeat in multiple places in the codebase, so I created this abstraction.
So that's not working right now, right?
There was a problem hiding this comment.
Since the rule is deprecated, my first choice would be to leave it as it is and remove the change to this rule, what do you think?
I think it would be a good idea to refactor the deprecation rules as well, to test whether the new functions work well.
There was a problem hiding this comment.
The playground doesn't work for me.
Try testing it with the following file:
<script>
/* eslint svelte/no-dynamic-slot-name: "error" */
const f = SLOT_NAME;
const SLOT_NAME = f;
</script>
<!-- ✓ GOOD -->
<slot name="good" />
<!-- ✗ BAD -->
<slot name={SLOT_NAME} />There was a problem hiding this comment.
Thanks, with the test case I was able to fix that rule as well :)
marekdedic
force-pushed
the
consistent-selector-style-infinite-loop-fix
branch
from
82b3e76 to
407a1c8
Compare
|
I don't think the new function makes maintenance easier. Instead, could you/we add a helper that prevents recursive calls to |
|
What do you think of this idea? #1344 |
|
In respect of #1344, this PR will be closed. Thank you! |
Hi, I added a fix for #1337 and the issues with
no-navigation-without-resolve(and its deprecated variants) mentioned in #1322I also checked the rest of the codebase for similar risky patterns and converted several places where it seemed useful or would clean up the code, please take a look and let me know if it makes sense.