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

Improve set() detection logic in no-side-effects rule to avoid false positives, catch missed cases, and check imports #914

Merged
merged 1 commit into from Aug 14, 2020

Conversation

bmish
Copy link
Member

@bmish bmish commented Aug 14, 2020

This improves the logic for detecting set() and setProperties() inside computed properties, since there were a lot of problems with it before.

Fixes some false positives including:

computed(function() { foo1.foo2.set('bar', 123); })
computed(function() { set('bar', 123); })

Catches additional violations including:

computed(function() { this.foo.set('bar', 123); }) // only `this.set()` detected before
computed(function() { set(this.foo, 123); }) // only `set(this, ...)` detected before

Also updates the rule to use the actual imported names for the set and setProperties functions.

Fixes #351
Fixes #377

@bmish bmish added the Bug label Aug 14, 2020
@bmish bmish changed the title Update no-side-effects rule to check all imports and avoid false positive Update no-side-effects rule to check all imports and avoid false positives Aug 14, 2020
@bmish bmish changed the title Update no-side-effects rule to check all imports and avoid false positives Improve set() detection logic in no-side-effects rule to avoid false positives, catch missed cases, and check imports Aug 14, 2020
… all imports, catch some missed cases, and avoid false positives

This improves the logic for detecting `set()` and `setProperties()` inside computed properties, since there were a lot of problems with it before.

Fixes some false positives including:

```js
computed(function() { foo1.foo2.set('bar', 123); })
```

```js
computed(function() { set('bar', 123); })
```

Catches additional violations including:

```js
computed(function() { this.foo.set('bar', 123); }) // only `this.set()` detected before
```

```js
computed(function() { set(this.foo, 123); }) // only `set(this, ...)` detected before
```

Also updates the rule to use the actual imported names for the `set` and `setProperties` functions.
@bmish bmish merged commit 384ccdd into ember-cli:master Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ember/no-side-effects should allow modifying locale variables no-side-effects false positive
1 participant