eslint-community / eslint-plugin-promise Goto Github PK

Looks like npm received a 3.0.0 release of this module, but no new tag and no new CHANGELOG entry has been added for this release.

A very common error in large project is to use Promise without importing it. This can be caught only when testing in an environment that does not declare global variable Promise (e.g. IE10). Therefore, I always require that developer import Promise to the local scope, e.g.,

import Promise from 'bluebird';
// or
let Promise = require('bluebird');

export default Promise.resolve(null);

No need check what is being imported as long as Promise variable is defined.

This rule must cause error only when Promise variable is used in the document.

I'm noob to Promises and this is why I REALLY love your plugin ... but I can't figure out why I get this error :
5:1 error Expected catch() or return promise/catch-or-return
On the following file:
sample.txt
Using the following .eslintrc:
eslintrc.txt

https://github.com/xjamundx/eslint-plugin-promise#always-catch

It is quiet rare that a promise chain itself would handle a failure. It is usually the top most promise node that must handle it, e.g. You wouldn't want your HTTP abstraction to fail the program. You'd want the program using the HTTP abstraction to handle whatever error arose.

Just an odd rule to have.

I get a false negative with the following code, complaining with "Expected catch() or return promise/catch-or-return":

postJSON("/smajobber/api/reportJob.json", { jobId: this.props.jobId})
                .then(() => this.setState({f: true}))
                .catch(() => this.setState({f: true}));

Where PostJSON return a Promise.

Seeing this from time to time, would be nice to warn about the unnecessary new in:

new Promise.resolve()
new Promise.reject()

It would be great to support the stage 2 proposal of the optional finally.
https://github.com/tc39/proposal-promise-finally

This feature is provided with this great shim:
https://github.com/es-shims/Promise.prototype.finally

I've seen different people choose different approaches here:

Using throw:

doThis().then(happyPath).catch(function(bad) {
   log(bad);
   throw bad;
});

Using Promise.reject():

doThis().then(happyPath).catch(function(bad) {
   log(bad);
   return Promise.reject(bad);
});

I'd like a rule to enforce one or the other.

I've seen this mistake a couple of times: avajs/ava#1119 (comment) Nested parens are hard to read and sometimes you wrap the wrong thing. Would be useful to have a rule the reported when you tried using more than one argument with Promise.resolve() or Promise.reject().

Error tooltip rule documentation links in tools like Atom linter-eslint are broken.

For example, https://github.com/xjamundx/eslint-plugin-promise#catch-or-return should be https://github.com/xjamundx/eslint-plugin-promise#rule-catch-or-return.

makePromise().then(thenHandler, catchHandler) is incorrectly considered correct by the always-catch plugin when allowThen: true is specified.

While the catchHandler will handle a rejection of the Promise returned by makePromise(), it will not handle any rejection that happens in the thenHandler; it needs to be handled by its own catch handler, which could be a .then(null /* | undefined */, catchHandler) according to allowThen: true.

it'd be nice to not have to explicitly include the rules in the config

Not sure what is the bug here, but I got this:

0:0  error  Each then() should return a value or throw  promise/always-return

and at the 0 line I have an import declaration:

import API from 'js/util/api';

The weird thing here is the I have this error only in one file.

similar to throw 'string', rejecting anything but an error should be discouraged

https://help.github.com/articles/creating-an-issue-template-for-your-repository/

For bug reports, it would be helpful for users to include:

• Node version
• ESLint version
• eslint-plugin-promise version
• ESLint config file contents
• actual behavior
• expected behavior

For the always-catch rule, I'd like to be able to consider this:

myPromise.then(doSomething, catchErrors);

as a valid catch, since lots of code that uses standard does that.

https://help.github.com/articles/creating-a-pull-request-template-for-your-repository/

This would be nice to have. Could include a checklist of things like:

• added tests
• did you update the documentation (if the changes you made require documentation updates)

Not sure what else right now, will take a look at some other open source projects for inspiration. Feedback/suggestions welcome, especially if you contribute to another project and work with a helpful PR template. 😄

Using return Promise.reject() is not wrapping, but just another way to reject the promise. I don't think this rule should be concerned about a stylistic issue.

If not, there should at least be an option to turn off requiring throw.

I personally favor:

myPromise.then(() => Promise.reject(new Error("bad thing")));

Over this:

myPromise.then(() => {
	throw new Error('bad thing');
});

I have a problem when displaying some errors from this plugin in atom.
I opened an issue for the linter ui.
But as described there it seems to be a problem with how the plugin reports errors.

Is this possible to fix from your side, because it makes the code hard to read?

Rule: consistent-promise-function

Ensure that if a function returns a Promise, it should consistently return a Promise. Instead of throwing an exception, it should return a rejected Promise

Valid

function myPromiseMethod(condition) {
  if (condition) {
    return Promise.resolve("some value");
  } else {
    return Promise.reject(new Error("Invalid result"));
  }
}

function myNormalMethod(condition, otherCondition) {
  if (condition) {
    return "some value";
  } else {
    if (otherCondition) {
      throw new Error("some problem");
    }
    return "some other value";
  }
}

Invalid

function badMethod(condition, otherCondition) {
  if (condition) {
    return Promise.resolve("some value");
  } else if (otherCondition) {
    return "other value";
  } else {
    throw new Error("Invalid condition");
  }
}

This gives error

return promise.resolve(1)
    .then((value) => {
        if (true) {
            return 1;
        } else {
            return 2;
        }
    });

Each then() should return a value or throw promise/always-return

Even though it's definitely returning either 1 or 2

Today there are many libraries that enhance the ECMA standard, like bluebird.
It will be very helpful if we can enforce only usage of the standard and not super-set.

The error each then() should return a value is reported even though the .then always returns a promise in the code below.

Apparently the logic for following return values can't evaluate an else clause

    return oracle.getPrices(c)
    .then(function(p) {
        if (p.price != 0) {
            return p.price.toNumber() / 1e6;
        } else {
            // XXX unavoidable race
            return 200;
        }
    });  

promise.then(it => {
    return;
});

=>

Cannot read property 'type' of null
TypeError: Cannot read property 'type' of null
    at EventEmitter.ReturnStatement (/Users/zloirock/wd/static/node_modules/eslint-plugin-promise/rules/no-return-wrap.js:25:28)
    at emitOne (events.js:101:20)
    at EventEmitter.emit (events.js:188:7)
    at NodeEventGenerator.enterNode (/Users/zloirock/wd/static/node_modules/eslint/lib/util/node-event-generator.js:40:22)
    at CodePathAnalyzer.enterNode (/Users/zloirock/wd/static/node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:608:23)
    at CommentEventGenerator.enterNode (/Users/zloirock/wd/static/node_modules/eslint/lib/util/comment-event-generator.js:97:23)
    at Controller.enter (/Users/zloirock/wd/static/node_modules/eslint/lib/eslint.js:925:36)
    at Controller.__execute (/Users/zloirock/wd/static/node_modules/eslint/node_modules/estraverse/estraverse.js:397:31)
    at Controller.traverse (/Users/zloirock/wd/static/node_modules/eslint/node_modules/estraverse/estraverse.js:501:28)
    at Controller.Traverser.controller.traverse (/Users/zloirock/wd/static/node_modules/eslint/lib/util/traverser.js:36:33)

Latest versions of eslint-plugin-promise, eslint, babel-eslint

The following code reports an issue:

try {
  Promise.all([
    this.promiseOne(),
    this.promiseTwo(),
  ]);
} catch (err) {
  // Handle error
}

(this.promiseOne and this.promiseOne are async functions)

But this passes:

Promise.all([
  this.promiseOne(),
  this.promiseTwo(),
]).catch(() => {
  // Handle error
});

I think the first configuration with the try/catch block should pass.

I can't wrap my head around on how to not nest my thens.

return database
    .read({id: 123})
    .then(function (object) {
        /* ... code that changes the object ... */
        return database
            .update(object) // resolves to true
            .then(() => object) // but we need to pass the object to the transport
    })
    .then(transport.respond)
    .catch(transport.error);

How can I not nest here?

If code has to support being parsed by IE8, it is necessary to avoid using the reserved word "catch" as an identifier, i.e.:

new Promise(function(resolve, reject) {
  ...
})
.then(...)
['catch'](function(err) { ... })

See this note in es6-promise for details.

Unfortunately, this pattern triggers the "promise/catch-or-return" rule, so one must consistently disable that rule to avoid having warnings. It would be ideal if ['catch'] would be interpreted the same as .catch. Is that possible, or is there a workaround (without using a transpiler to rewrite the code)?

I've heard that catch() should re-throw the error or return Promise.reject(err) anyway familiar with this? It's so you don't break the promise chain.

It might be a good idea to merge zertosh/eslint-plugin-no-async-without-await into this plugin. Seems like a good fit. Thoughts?

Similar to: http://bluebirdjs.com/docs/warning-explanations.html

Examples:

Promise.resolve(1)
  .then(Promise.resolve(2)) // This is ignored, probably not what the developer intended
  .then(console.log) // 1

const errorHandler = console.warn;
Promise.reject('Catch me if you can').catch(errorHandler()) // should have been `errorHandler` without parenthesis 

It wouldn't be possible to cover this completely with static code analysis I guess (we would have to know that console.warn isn't a function that returns another function), but the first example should be detectable at least?

ESLint Version: 3.8.1
Promise Plugin Version: 3.0.0

Configuration

{
  "plugins": ["promise"],
  "rules": {
    "promise/always-return": "error"
  }
}

Minimal example

something.then(function (a) {
  return a ? 'answer' : 42;
});

Expected: No error
Actual: error Each then() should return a value or throw

This innocently looking assignment:

const FAIL = Promise.reject();

causes UnhandledPromiseRejectionWarning if FAIL is not used...

This code

performRequest().then(() => console.debug('update successful'), err => console.error(err))

emits Expected catch() or return (promise/catch-or-return)

Willing to make a PR for recommended settings for this plugin. Would it be welcome?

See #48 (comment)

return inside finally is a bit misleading because the return value is not the new resolved value:

// this promise has a resolved value of 1
Promise.resolve(1).finally(() => {
  return 2;
});

see: https://github.com/tc39/proposal-promise-finally

I have the following code:

const Promise = require("bluebird");
const db = require("./db");

// Connect to DB and apply migrations
// or retry calling itself every second if can't connect
module.exports = function connect(params) {
    return db
        .connect(params)
        .then(
            () => db.applyMigrations(),
            () => Promise.delay(1000).then(() => connect(params))
        );
};

I get the "Avoid nesting promises" warning for line

            () => Promise.delay(1000).then(() => connect(params))

I can't think of a way to rewrite this code without nesting.
I think the "no-nesting" rule should allow using .then() in the reject function.

See eslint/eslint#5760

Rule to warn when wrapping a value returned from a Promise handlerFn in Promise.resolve or Promise.reject

something.then(function () {
  // `Promise.resolve` is redundant, just use `return val;`
  return Promise.resolve(val);
});

// and

something.then(function () {
  // `Promise.reject` is redundant, just use `throw val;`
  return Promise.reject(val);
});

I think it would be nice to add some information to help potential contributors set up their environments, tips for running tests, etc.

For example, I set up a Node 4 environment for development (since that is the primary testing environment on Travis).

I also find it easier to watch a specific test when developing a new rule so I am only running the tests pertaining to my lint rule: mocha test/my-rule.js --watch. Perhaps that could be something to include in this document?

I'm not sure what sort of guidelines to include based on the past experience of this project, so I'd appreciate feedback from maintainers and other contributors. I'd be glad to propose a CONTRIBUTING.md PR to get more conversation started, if that would help. 👍

I could just disable the rule but I'd like to understand why this error is being thrown. Thanks!

Below is my code (the entire file). The error underlines const.

const settings = {};
module.exports = { settings };

Here is my .eslintrc

{
  "env": {
    "node": true,
    "browser": true,
    "es6": true
  },
  "extends": "eslint:recommended",
  "parserOptions": {
    "ecmaFeatures": {
      "experimentalObjectRestSpread": true,
      "jsx": true
    }
  },
  "plugins": [
    "react",
    "node"
  ],
  "rules": {
    "indent": ["error", 2, { "SwitchCase": 1 }],
    "linebreak-style": ["error", "unix" ],
    "quotes": ["error", "single"],
    "semi": ["error", "always"],
    "arrow-parens": ["off"],
    "consistent-return": "off",
    "comma-dangle": "off",
    "no-console": "off",
    "no-use-before-define": "off",
    # "no-underscore-dangle": "off",
    "generator-star-spacing": "off",
    "promise/param-names": 2,
    "promise/always-return": 2,
    "promise/catch-or-return": 2,
    "promise/no-native": 0,
    "react/jsx-no-bind": "off",
    "react/jsx-filename-extension": ["error", { "extensions": [".js", ".jsx"] }],
    "react/prefer-stateless-function": "off",
    "react/forbid-prop-types": "off",
    "flowtype-errors/show-errors": 2
  }
}

It is impossible to keep up with fixes/ new rules added without a change log. A good example of a maintained CHANGELOG is https://github.com/yannickcr/eslint-plugin-react/blob/master/CHANGELOG.md.

I want to add eslint-plugin-promise to an open-source project.

When I ran npm run eslint **/*.js locally, I received:

/Users/pulkitsinghal/dev/shoppinpal/loopback-connector-elasticsearch/lib/automigrate.js
  47:18  error  Each then() should return a value or throw  promise/always-return

which points to the code in line 47 of this branch: return Promise.map()

1. Is it reasonable to expect that the linting process should treat return Promise.map() as a valid returned value?
2. Or since .map() is bluebird specific, perhaps its not a reasonable expectation?

Yielding promises is common when using tj/co or similar constructs.
For example:

co(function * g () {
  let status = yield getResult().then((result) => result.status)
})

Sometimes a callback isn't a "nodeback", as with middleware:

function middleware(req, res, next) {
    return promise(req)
        .then(function (result) {
            res.send(result);
            return next();
        })
        .catch(next);
}

It is impossible to use noedify in this case, so it should be possible to add "next" to exceptions.

See: eslint/eslint#5761

Rule to throw when the identify function is used as the success handler of a Promise or the "throw function" is used as the failure handler of a Promise.

// The identity function is the default success handler for a promise, it can be ommitted
something.then(function (value) {
  return value;
});

// and

// propagating the error is the default error handler fro a promise, it can be ommitted

something.then(null, function (err) {
  throw err;
});

// or

something.catch(function (err) {
  throw err;
});

It helps users to understand how promise values propagate.

Usually when people write .catch(err => { throw err; }) they intend to throw the error into the next tick to make sure it's reported, not to add a no-op to the end of their promise chain.

promise.then(fn=>{
  return fn && fn();
})

This code should not throw the always-return error

I'm using eslint 3.8.1. When running eslint, i get the following error:

Cannot set property 'good' of undefined
TypeError: Cannot set property 'good' of undefined
    at EventEmitter.markCurrentBranchAsGood (/Users/amila/Documents/Projects/parallel.js/node_modules/eslint-plugin-promise/rules/always-return.js:75:52)

In a local branch, I played around with integrating eslint-plugin-eslint-plugin into this codebase, and noticed a lot of reports of no-deprecated-report-api.

For example, we should change instances of this:

https://github.com/xjamundx/eslint-plugin-promise/blob/2271a7fd162e9fa37a5f68bdca2edc4c38105f00/rules/avoid-new.js#L10

to this:

context.report({ node, message: 'Avoid creating new promises.' })

Dependent on #63 being resolved, since eslint-plugin-eslint-plugin requires a peer dependency of ESLint >=4

Taking the example from the linked article and extending it a bit:

getUserByName('nolan').then(function (user) {
  if (user.isLoggedOut()) {
    throw new Error('user logged out!'); // throwing a synchronous error!
  }
  if (inMemoryCache[user.id]) {
    return inMemoryCache[user.id];       // returning a synchronous value!
  }
  return getUserAccountById(user.id);    // returning a promise!
}).then(function finalThen(userAccount) {
  console.log(userAccount);
}).catch(function (err) {
  console.error(err);
});

There's no need to return anything in finalThen. Would it be possible to ignore these cases (in the last then, when the promise isn't returned or used in any way)?

The following would become valid:

myPromise.then(function(val) {});

The following would remain invalid:

myPromise.then(function(val) { /* missing return here */ }).then(function(val2) { /* no error here */ });
var p = myPromise.then(function(val) {});
return myPromise.then(function(val) {});

Need a new rule to prefer new Promise over Promise.defer/pending.

Could be an extension of promise/avoid-new because it is the opposite behaviour.

Should stop:

var Q = Promise.pending();
return Q.promise;

and

var Q = Promise.defer();
// stuff with Q.resolve() or Q.reject()
return Q.promise;

and the --fix would replace

var Q = Promise.defer();
// stuff with Q.resolve() or Q.reject()
return Q.promise;

with

return new Promise(resolve, reject) {
// replacing Q.resolve/Q.reject with just resovle/reject
});