feat: add svelte/Infinite-reactive-loop rule
#332
Conversation
🦋 Changeset detectedLatest commit: 336dfa1 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 |
baseballyama
force-pushed
the
feature/infinite-reactive-loop
branch
from
d60b044 to
7f84429
Compare
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
| } | ||
|
|
||
| $: (() => { | ||
| // TODO: This is a false positive. we need to check if the `then` or `catch` is `Promise` object's function. |
There was a problem hiding this comment.
I need your help!
I wanted to check that then or catch came from Promise type, but I couldn't find a way.
Could you please tell me?
There was a problem hiding this comment.
I think it's difficult to strictly check whether it's a Promise object or not. However, it's mostly okay to consider objects with then and catch methods that take functions as parameters to be Promises.
Other popular plugins also judge that way.
- https://github.com/typescript-eslint/typescript-eslint/blob/4ab9bd7f7c1812df4371d1fd3202969c1039e8a7/packages/eslint-plugin/src/rules/no-floating-promises.ts#L234
- https://github.com/eslint-community/eslint-plugin-promise/blob/6d046e6033d43d2c023966b33a7fabc1438899c2/rules/lib/is-promise.js#L14
It's clearly a false positive in this test case, but I think we can consider that people don't do it.
baseballyama
changed the title
svelte/Infinite-reactive-loop rulesvelte/Infinite-reactive-loop rule
|
@ota-meshi What do you think about the current specification and implementation? Do you think there are TODOs? |
|
I misunderstood that this PR was still in the work in progress 😓. I will make time to check this PR. So please wait 🙏 |
src/rules/infinite-reactive-loop.ts
Outdated
|
|
||
| // Promise.then() or Promise.catch() is called. | ||
| if (isPromiseThenOrCatchBody(node)) { | ||
| isSameMicroTask = true |
There was a problem hiding this comment.
I think we can remove the duplicate code by keeping the node when it changes to isSameMicroTask = false in enterNode() so that when the same node comes, isSameMicroTask = true. What do you think?
There was a problem hiding this comment.
Good catch. I removed the duplicate code.
Co-authored-by: Yosuke Ota <[email protected]>
Co-authored-by: Yosuke Ota <[email protected]>
Co-authored-by: Yosuke Ota <[email protected]>
close: #299