Skip to content

no-effect-override rule #4

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

no-effect-override rule #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,6 @@
"wrap-regex": 0,
// Legacy
"max-depth": 0,
"max-len": [2, 120],
"max-params": 0,
"max-statements": 0,
"no-plusplus": 0
Expand Down
51 changes: 49 additions & 2 deletions docs/rules/yield-effects.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,57 @@
**fixable**<br>
The `--fix` option on the command line automatically fixes problems reported by this rule.

This rule ensures that no `redux-saga` effects are (accidentally) overriden
by a function with the same name in the scope and are properly `yield`'ed.

This rule ensures that all `redux-saga` effects are properly `yield`'ed.
Overriding or not `yield`'ing an effect might result in unexpected saga control flow behaviour.

Not `yield`'ing an effect might result in strange control flow behaviour.

## Examples

### Overriding effects

```es6
import { take, put as sagaPut } from "redux-saga"

// good
function* good() {
yield take("action")
}

// good
function* good() {
function put() {}

yield sagaPut("action")
}

// good
function put() {
}

function* good() {
yield sagaPut("action")
}


// bad
function* bad() {
function take() {}

yield take("action")
}

// bad
function take() {
}

function* bad() {
yield take("action")
}
```

### Not `yield`'ing effects:

```es6
import { take } from "redux-saga"
Expand Down
51 changes: 51 additions & 0 deletions lib/rules/yield-effects.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ module.exports = {
},

create: function(context) {
var self = this
var effectNames = []
return {
"ImportDeclaration": function(node) {
Expand All @@ -29,6 +30,14 @@ module.exports = {
"CallExpression": function(node) {
var callee = node.callee
if (effectNames.indexOf(callee.name) !== -1) {
var scopeVariable = self.variableInScope(context, callee.name)
if (scopeVariable) {
context.report({
node: scopeVariable.identifiers[0],
message: scopeVariable.name + " overrides redux-saga effect with the same name"
})
return
}
var sourceCode = context.getSourceCode()
var previousTokens = sourceCode.getTokensBefore(node, 1)
if (previousTokens[0].value !== "yield") {
Expand All @@ -43,5 +52,47 @@ module.exports = {
}
}
}
},

/**
* Returns the variable that overrides an ImportBinding
*
* Contain a patch for babel-eslint to avoid https://github.com/babel/babel-eslint/issues/21
*
* @param {Object} context The current rule context.
* @param {String} name The name of the variable to search.
* @returns {Object} the found variable or null if none was found.
*
* based on:
* https://github.com/yannickcr/eslint-plugin-react/blob/442d20b6e1bb5daeacd5234f253f963af90cc3e4/lib/util/variable.js#L61-L86
*/
variableInScope: function(context, name) {
var scope = context.getScope()
var variables = scope.variables

while (scope.type !== "module") {
scope = scope.upper
variables = scope.variables.concat(variables)
}
if (scope.childScopes.length) {
variables = scope.childScopes[0].variables.concat(variables)
if (scope.childScopes[0].childScopes.length) {
variables = scope.childScopes[0].childScopes[0].variables.concat(variables)
}
}

var result
variables.some(function(variable) {
if (variable.name === name) {
if (variable.defs.some(function(def) {
return def.type !== "ImportBinding"
})) {
result = variable
}
}
return !!result
})

return result
}
}
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
},
"homepage": "https://github.com/pke/eslint-plugin-redux-saga",
"scripts": {
"test": "mocha --recursive --colors",
"start": "mocha --watch --recursive"
"pretest": "eslint lib test",
"test": "mocha --recursive --colors"
},
"peerDependencies": {
"eslint": "2.x - 3.x",
Expand Down
4 changes: 2 additions & 2 deletions test/lib/rules/no-yield-in-spec.js → test/lib/rules/no-yield-in-race-spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ ruleTester.run("no-yield-in-race", rule, {
{
code: "function* test() { yield race({ posts: yield call(fetchApis) }) }",
output: "function* test() { yield race({ posts: call(fetchApis) }) }",
errors: [{message: "yield not allowed inside race: posts"}]
errors: ["yield not allowed inside race: posts"]
},
{
code: "function* test() { yield race({ watchers: yield [call(watcher1), call(watcher2)] }) }",
errors: [{message: "yield not allowed inside race: watchers"}],
errors: ["yield not allowed inside race: watchers"],
output: "function* test() { yield race({ watchers: [call(watcher1), call(watcher2)] }) }"
}
]
Expand Down
120 changes: 119 additions & 1 deletion test/lib/rules/yield-effects-spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,62 @@ function buildTest(imports, code) {
return result
}

var ERRORS = {
overrides: "take overrides redux-saga effect with the same name",
noyield: "take effect must be yielded"
}

ruleTester.run("yield-effects", rule, {
valid: [
{
code: "import { take } from 'redux-saga';"
+ "const sagaTake = take;"
+ "export default function* saga() {"
+ " yield sagaTake();"
+ "}"
},
{
code: "import { take } from 'redux-saga';"
+ "export default function* saga() {"
+ " const sagaTake = take;"
+ " yield sagaTake();"
+ "}"
},
{
code: "import { take as sagaTake } from 'redux-saga';"
+ "const sagaTakeCreator = sagaTake;"
+ "export default function* saga() {"
+ " yield sagaTakeCreator();"
+ "}"
},
{
code: "import { take as sagaTake } from 'redux-saga';"
+ "export default function* saga() {"
+ " const sagaTakeCreator = sagaTake;"
+ " yield sagaTakeCreator();"
+ "}"
},
{
code: buildTest("import { take } from 'redux-saga'", "yield notAnEffect()")
},
{
code: buildTest("import { take as sagaTake } from 'redux-saga'", "const take = () => {}; yield sagaTake()")
},
{
code: buildTest("import { take as sagaTake } from 'redux-saga'", "function take() {} yield sagaTake()")
},
{
code: buildTest("import { take as sagaTake } from 'redux-saga'", "var take = function() {}; yield sagaTake()")
},
{
code: buildTest("import { take as sagaTake } from 'redux-saga'; const take = () => {};", "yield sagaTake()")
},
{
code: buildTest("import { take as sagaTake } from 'redux-saga'; function take() {}", "yield sagaTake()")
},
{
code: buildTest("import { take as sagaTake } from 'redux-saga'; var take = function() {};", "yield sagaTake()")
},
{
code: buildTest("import { take } from 'redux-saga'", "yield take('ACTION')")
},
Expand All @@ -24,10 +78,74 @@ ruleTester.run("yield-effects", rule, {
}
],
invalid: [
{
code: "import { take } from 'redux-saga';"
+ "export default function* saga() {"
+ " const sagaTakeEffect = take();"
+ " yield sagaTakeEffect;"
+ "}",
errors: [ERRORS.noyield]
},
{
code: "import { take as sagaTake } from 'redux-saga';"
+ "export default function* saga() {"
+ " const sagaTakeEffect = sagaTake();"
+ " yield sagaTakeEffect;"
Copy link

Choose a reason for hiding this comment

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

Do we allow yielding something that is not an effect or array of effects?

BTW, the test for yielding array of effects is missing.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think general function generator linting should be done in a separate plugin or eslint core.

I'll add the other tests for arrays

Copy link

Choose a reason for hiding this comment

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

It's a valid piece of code, you can generally yield any value from a generator. The question is, do we allow that for sagas (the files where the redux-saga module is imported)?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I would say thats for another linter plugin that specifically deals with generators. The case tested above seems very unlikely to happen in a real program.

Copy link

Choose a reason for hiding this comment

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

Another plugin that would deal with generators should treat this code as valid, because it is. You can yield anything.

But, when using redux-saga, you should only yield the return values of the effect creators (or arrays of such).

So, it's this eslint-plugin-redux-saga's responsibility to limit the usage of generators to only yield redux-saga effects when a saga file is detected (by assuming files which import redux-saga as files that declare sagas, and generators inside them are sagas).

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ok, I agree. So which additional tests / checks you suggesting to complete this rule?

Copy link

Choose a reason for hiding this comment

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

Sorry for the delay.

I would test for the patterns described in the redux-saga documentation, starting from here: http://yelouafi.github.io/redux-saga/docs/basics/UsingSagaHelpers.html and here: http://yelouafi.github.io/redux-saga/docs/advanced/RunningTasksInParallel.html

Note that the effect creators are imported from redux-saga/effects, but the helpers which are to be yield*ed, not yielded, are imported from redux-saga itself:

import { call, put } from 'redux-saga/effects'
import { takeEvery } from 'redux-saga'

Note that yielding an array of calls to effect creators is also valid:

// correct, effects will get executed in parallel
const [users, repos]  = yield [
  call(fetch, '/users'),
  call(fetch, '/repos')
]

Copy link
Owner Author

Choose a reason for hiding this comment

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

Will be able to work on that more now.

+ "}",
errors: ["sagaTake effect must be yielded"]
},
// overrides
{
code: buildTest("import { take } from 'redux-saga'", "const take = () => {}; yield take()"),
errors: [ERRORS.overrides]
},
{
code: buildTest("import { take } from 'redux-saga'; const take = () => {}", "yield take()"),
errors: [ERRORS.overrides]
},
{
code: buildTest("import { take } from 'redux-saga'; var take = function() {}", "yield take()"),
errors: [ERRORS.overrides]
},
{
code: buildTest("import { take } from 'redux-saga'; const take = function() {}", "yield take()"),
errors: [ERRORS.overrides]
},
{
code: buildTest("import { take } from 'redux-saga'; function take() {}", "yield take()"),
errors: [ERRORS.overrides]
},
{
code: buildTest("import { take } from 'redux-saga'", "const take = () => {}; take()"),
errors: [ERRORS.overrides]
},
{
code: buildTest("import { take } from 'redux-saga'; const take = () => {}", "take()"),
errors: [ERRORS.overrides]
},
{
code: buildTest("import { take } from 'redux-saga'; var take = function() {}", "take()"),
errors: [ERRORS.overrides]
},
{
code: buildTest("import { take } from 'redux-saga'; const take = function() {}", "take()"),
errors: [ERRORS.overrides]
},
{
code: buildTest("import { take } from 'redux-saga'; function take() {}", "take()"),
errors: [ERRORS.overrides]
},

// no yield
{
code: buildTest("import { take } from 'redux-saga'", "take('ACTION')"),
output: buildTest("import { take } from 'redux-saga'", "yield take('ACTION')"),
errors: [{message: "take effect must be yielded"}]
errors: [ERRORS.noyield]
},
{
code: buildTest("import { take as sagaTake } from 'redux-saga'", "sagaTake('ACTION')"),
output: buildTest("import { take as sagaTake } from 'redux-saga'", "yield sagaTake('ACTION')"),
errors: ["sagaTake effect must be yielded"]
}
]
})