Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Adding a new no-ignored-unsubscribe rule. #592

Merged
merged 4 commits into from
Oct 4, 2023

Conversation

moufmouf
Copy link
Contributor

@moufmouf moufmouf commented Oct 3, 2023

Hey @ota-meshi

First of all, thanks for this package, it's really a life saver!

I would like here to propose a new rule for the eslint Svelte plugin: no-ignored-unsubscribe.

This rule fails if an "unsubscriber" returned by call to subscribe() is neither assigned to a variable or property nor passed to a function.

One should always unsubscribe from a store when it is no longer needed. Otherwise, the subscription will remain active and constitute a memory leak. This rule helps to find such cases by ensuring that the unsubscriber (the return value from the store's subscribe method) is not ignored.

I'm actually writing this rule after being bite by such an issue => https://github.com/workadventure/workadventure/pull/3527/files

About the code:

The rule is heavily inspired by a similar rule in eslint-plugin-rxjs: https://github.com/cartant/eslint-plugin-rxjs/blob/main/docs/rules/no-ignored-subscription.md

I did simplify the code a bit. As it is now, the rule will generate a few false positives (it is triggered on all method named subscribe, whether the underlying object is a store or not). In practice, I think this is acceptable.

There is alas no simple way to check the type of the underlying object. The RxJS rule relies on analyzing the code to try to find the type of the object. I tested a similar approach. It has a number of false negatives, and the code is really more complex as it relies on doing some type testing with typescript-eslint (as described here).

Let me know what you think!

This rule fails if an "unsubscriber" returned by call to `subscribe()` is neither assigned to a variable or property or passed to a function.

One should always unsubscribe from a store when it is no longer needed. Otherwise, the subscription will remain active and constitute a **memory leak**.
This rule helps to find such cases by ensuring that the unsubscriber (the return value from the store's `subscribe` method) is not ignored.
@changeset-bot
Copy link

changeset-bot bot commented Oct 3, 2023

🦋 Changeset detected

Latest commit: 0c2dcbf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
eslint-plugin-svelte Minor

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

@moufmouf moufmouf changed the title Adding a new no-ignored-unsubscribe rule. feat: Adding a new no-ignored-unsubscribe rule. Oct 3, 2023
docs/rules/no-ignored-unsubscribe.md Outdated Show resolved Hide resolved
docs/rules/no-ignored-unsubscribe.md Outdated Show resolved Hide resolved
src/rules/no-ignored-unsubscribe.ts Outdated Show resolved Hide resolved
src/rules/no-ignored-unsubscribe.ts Outdated Show resolved Hide resolved
@moufmouf
Copy link
Contributor Author

moufmouf commented Oct 4, 2023

Hey @ota-meshi!

Thanks a lot for this very quick and thorough review. I'm impressed!
I applied your suggestions and made the requested changes. Let me know if something is missing!

Copy link
Member

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for suggesting and implementing the rule!

@ota-meshi ota-meshi merged commit 1fe38d7 into sveltejs:main Oct 4, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants