Require a newline after each call in a method chain (newline-per-chained-call)

[HR]

Hi @feross,
I would like to discuss adding a new rule: always break promise chains. Referring to the ES6 Promises (defined by the ECMA spec), breaking up promise chains makes the code much more readable. Consider the following.

So instead of writing a promise chain like this

getURL(URL).then((res) => {
    return processPromise(res)
  }).then((res) => {
    return processMorePromise(res)
  }).catch((err) => {
    console.error(err)
  })

It is far more cleaner, prettier and readable to break the promise chain like this with a new line and indentation.

getURL(URL)
  .then((res) => {
    return processPromise(res)
  })
  .then((res) => {
    return processMorePromise(res)
  })
  .catch((err) => {
    console.error(err)
  })

I hope that you agree with me and it can become part of the standard rules.

Many thanks.

[dcousens]

@HR is there an eslint rule for this?

[HR]

@dcousens I checked and couldn't find one. So, I guess not.

[dcousens]

Probably better off making your case at eslint first, then coming back here 👍 , though, I don't know if this rule would catch on, depends on what the majority of users are already doing.

[HR]

@dcousens I think that this rules goes perfectly well with the idea of writing "code that you find beautiful" which is what standard is about from what I understand (not that sure about eslint). I think you would agree with me that code with this rule is much more readable.

[dcousens]

@HR I don't use promises [yet™]

I do use chains though, and I don't use the fancy syntax you promote.
Then again, I don't have a strong opinion on it except that will break existing code.

[HR]

@dcousens Ok. I'll try making a case at eslint but I believe that this issue should get more exposure and the input of more devs before an informed decision can be made.

[grantcarthew]

I don't see that this should be a rule. It's more of a choice for the dev. I use the first example above. I don't see the proposed rule to be that much better.

[HR]

@grantcarthew then how is a "Space after keywords" or a "Space after function name" that much better? The proposed rule makes the code more readable than adding a space after keywords does. Wouldn't you agree?

[grantcarthew]

I just tried it out on my RethinkDB queries. It seems better. Still think it is a choice though.

[HR]

@grantcarthew so is adding a semicolon! It helps create "code that you find beautiful" because it is cleaner which is why you exclude the semicolon.

[grantcarthew]

I see your point @HR. I'm no expert though. If you read any tutorial online about Promises they are using the first example syntax above.

[dcousens]

@HR probably more importantly, is that there is "less of a choice"... again, find an eslint rule, and if its too ambiguous either way (aka, 50/50 for standard users), then I can't see it being enforced.

[MrZhangFengfeng]

I like yours @HR

[feross]

There's no eslint rule for this specific of a scenario, even in eslint-plugin-promise, however the newline-per-chained-call rule is closely related, and it's a pretty good idea:

newline-per-chained-call: ["error", { "ignoreChainWithDepth": 2 }]

Thoughts?

[dcousens]

@feross I'd see what the damage is first 😆

[HR]

@feross That looks good 👍 . @dcousens Just need to check for conflicting rules.

[sonicdoe]

Since this rule is still being discussed, I’d like to question the indentation of promise chains. Especially for larger chains, it can lead you into thinking that you forgot to close a code block:

function doSomething () {
  return getURL(URL)
    .then((res) => {
      return processPromise(res)
    })
    .then((res) => {
      return processMorePromise(res)
    })
    .catch((err) => {
      console.error(err)
    })
}

This has also been discussed in beautify-web/js-beautify#482, where @brandondrew put it quite well:

[the indented syntax] seems to reflect that the then() in some sense has a parent-child relationship with Promise.resolve(), but if that is true, then so does each subsequent then() have that relationship with the previous then(). Of course there really is no such parent-child relationship, and indenting as if there were all the way down would make a grand mess, so no one does it.

Anyway, this probably won’t pass for the majority of standard users, so I can understand why this might not be a good idea.

For what it’s worth, I’d really like to see such a rule – regardless of which syntax ends up being standardized – because it would be one less thing to think about.

[dcousens]

Agreed with @sonicdoe that that indentation has thrown me off when reading others code.
I've sometimes opted for no indentation:

array
.filter
.map
.reduce

But, I honestly don't have a solid opinion on either.

[feross]

I usually indent anytime an expression extends onto multiple lines, e.g.

if (condition1 && condition2 && condition3 &&
  condition4 && condition5) { // <-- indented here
  console.log('test')
}

The indention signals intent: the indented line can't be understood without looking at the previous line.

That said, I do appreciate the point you've made. Arguably the dots at the beginning of each method can signal the same thing. And yet, this looks like a bug to me:

arr
.map()
.filter()
console.log('test') // is this part of the previous chain?

Whereas, this does not:

arr
  .map()
  .filter()
console.log('test') // this is clearer

[dcousens]

@feross I don't mind either, I think your logic:

The indention signals intent: the indented line can't be understood without looking at the previous line.

Is sound, and is readable.
The dots do work in the same way too, but, are inconsistent.

[carmelocatalfamo]

Hi, indentation rules can be customizable (or can be disabled)?
I love this standard but i think that promises chained on same line (vertically) are more readable.
Actually I have disabled standard plugin on my code editor: there is a method to customize this rule?

From

getURL(URL)
  .then((res) => {
    return processPromise(res)
  })
  .then((res) => {
    return processMorePromise(res)
  })
  .catch((err) => {
    console.error(err)
  })

To

getURL(URL)
.then((res) => {
  return processPromise(res)
})
.then((res) => {
  return processMorePromise(res)
})
.catch((err) => {
  console.error(err)
})

[feross]

@carmelocatalfamo Sorry, there's no way to customize standard. You can try standardx if you want to break out and make tweaks, though it's probably not worth it IMO.

[feross]

After further consideration, I don't think the rule to require a newline after each call in a method chain (newline-per-chained-call) makes sense.

Some places where it fails:

Database migration files:

exports.up = knex => {
  return knex.schema
    .table('midis', table => {
      table.integer('plays').defaultTo(0).notNullable().index()
    })
}

D3 visualizations where you want to place methods on the same line for clarity:

    d3.select(info.selector)
      .datum(info.data)
      .transition().duration(500)
      .call(chart)

And sometimes it's just fine to put everything on one line:

const first = crypto.createHash('sha1').update('first').digest()

Even by adding a "magic number" to the rule (which we usually don't do because it's confusing) in the form of "newline-per-chained-call": ["error", { "ignoreChainWithDepth": 2 }], we end up with too many unpredictable failures.

1..162
# tests 162
# pass  139
# fail  23