Repository: eslint-community/eslint-plugin-promise
Pull request #518 (2024-07-26 12:26:21)

feat(always-return): add `ignoreAssignmentVariable` option
Adds `ignoreAssignmentVariable` as discussed in https://github.com/eslint-community/eslint-plugin-promise/pull/518#issuecomment-2260068048

brettz9: I could still be persuaded (or overruled), but I'm personally a little wary of such a change as this.

I think it's safer for the default to annoy rather than accidentally avoid warnings. In the case of libraries at least, one typically does want to always return.
gurgunday: I’d simply argue that, since this plugin is not eslint-plugin-n (and therefore not node specific), we should definitely take into account browser usage where these non blocking async globalThis assignments are common

I think the eslint-community plugins’ recommended presets should only error for usages that are incorrect ~100% of the time

For instance the chained .then usage where a .then in the middle does not return anything is most definitely not a correct usage, but for the last one we can argue cases like these, so it’d be more consistent to relax this to not fail here or maybe just warn
aladdin-add: just want to point out: it should be a breaking change to change a rule's default option.
gurgunday: Yeah I wish I'd opened the PR earlier to maybe target the v7 release
scagood: I am not sure I agree here, as enabling `ignoreLastCallback` will void the point of the check in a lot of cases. :thinking:

In fact I would prefer to remove the rule from the recommended config that making this change.

---

A different option could be allowing assignment to specific variables?
eg `ignoreAssignmentVariable: ["global.window"]`

Would this work for you?
gurgunday: > A different option could be allowing assignment to specific variables?

This makes a lot of sense and I didn’t know it was possible! Let me take a look
gurgunday: @scagood, @brettz9m @aladdin-add, can you take a look at this?
gurgunday: Hey everyone, so I made the option recursive, which means it allows for assignments to nested properties like `window.a.b` or `window["test"][0]`

I had to use string manipulation since I'm not familiar with how else to achieve this, but it looks sound to me as a property can either be computed or not, and we take both into account
gurgunday: Cc @scagood @brettz9
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/eslint-community/eslint-plugin-promise/pull/518?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eslint-community) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 100.00%. Comparing base [(`bbcfcbf`)](https://app.codecov.io/gh/eslint-community/eslint-plugin-promise/commit/bbcfcbf85ef97cba58f4fe4b9f46c907e04bc5a6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eslint-community) to head [(`dfef58e`)](https://app.codecov.io/gh/eslint-community/eslint-plugin-promise/commit/dfef58e50576b96cc5108bfa6e7f45283352f425?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eslint-community).
> Report is 47 commits behind head on main.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff            @@
##              main      #518   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        26    +1     
  Lines          649       721   +72     
  Branches       250       278   +28     
=========================================
+ Hits           649       721   +72     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/eslint-community/eslint-plugin-promise/pull/518?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eslint-community).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eslint-community).

gurgunday: PTAL @brettz9 @scagood 
gurgunday: Okay, so https://github.com/eslint-community/eslint-plugin-promise/pull/518#pullrequestreview-2235490839 made a good point - we need to take into account both variations (computed or not computed) for each level of a passed variable name (i.e., if `window.a.b` is passed as ignoreAssignmentVar, `window.a.b` `window.['a'].['b']` and `window.a['b']` should all pass)

It's not hard but it requires a little more complex logic

To keep the quality high and implementation correct, what I propose is that we start with 1 level of nesting - just `window` or `globalThis` - and we go from there to then add this functionality
gurgunday: PTAL with the [current setting](https://github.com/eslint-community/eslint-plugin-promise/pull/518/files#diff-f3971ff31439613bbaac29e9057328d29226c01f885835eec5e1be864c3957afR197)
scagood: @brettz9 Do you want to take another look at this PR?
brettz9: Sorry, is anyone else available? I've come into other work which is keeping me busy...

Patch:
diff --git a/docs/rules/always-return.md b/docs/rules/always-return.md
index d778771..81899d9 100644
--- a/docs/rules/always-return.md
+++ b/docs/rules/always-return.md
@@ -49,9 +49,9 @@ myPromise.then((b) => {
 
 ##### `ignoreLastCallback`
 
-You can pass an `{ ignoreLastCallback: true }` as an option to this rule to the
-last `then()` callback in a promise chain does not warn if it does not have a
-`return`. Default is `false`.
+You can pass an `{ ignoreLastCallback: true }` as an option to this rule so that
+the last `then()` callback in a promise chain does not warn if it does not have
+a `return`. Default is `false`.
 
 ```js
 // OK
@@ -92,3 +92,43 @@ function foo() {
   })
 }
 ```
+
+##### `ignoreAssignmentVariable`
+
+You can pass an `{ ignoreAssignmentVariable: [] }` as an option to this rule
+with a list of variable names so that the last `then()` callback in a promise
+chain does not warn if it does an assignment to a global variable. Default is
+`["globalThis"]`.
+
+```js
+/* eslint promise/always-return: ["error", { ignoreAssignmentVariable: ["globalThis"] }] */
+
+// OK
+promise.then((x) => {
+  globalThis = x
+})
+
+promise.then((x) => {
+  globalThis.x = x
+})
+
+// OK
+promise.then((x) => {
+  globalThis.x.y = x
+})
+
+// NG
+promise.then((x) => {
+  anyOtherVariable = x
+})
+
+// NG
+promise.then((x) => {
+  anyOtherVariable.x = x
+})
+
+// NG
+promise.then((x) => {
+  x()
+})
+```
diff --git a/rules/always-return.js b/rules/always-return.js
index 093ea33..7d9784b 100644
--- a/rules/always-return.js
+++ b/rules/always-return.js
@@ -124,6 +124,36 @@ function peek(arr) {
   return arr[arr.length - 1]
 }
 
+/**
+ * Gets the root object name for a MemberExpression or Identifier.
+ * @param {Node} node
+ * @returns {string | undefined}
+ */
+function getRootObjectName(node) {
+  if (node.type === 'Identifier') {
+    return node.name
+  }
+  // istanbul ignore else (fallback)
+  if (node.type === 'MemberExpression') {
+    return getRootObjectName(node.object)
+  }
+}
+
+/**
+ * Checks if the node is an assignment to an ignored variable.
+ * @param {Node} node
+ * @param {string[]} ignoredVars
+ * @returns {boolean}
+ */
+function isIgnoredAssignment(node, ignoredVars) {
+  if (node.type !== 'ExpressionStatement') return false
+  const expr = node.expression
+  if (expr.type !== 'AssignmentExpression') return false
+  const left = expr.left
+  const rootName = getRootObjectName(left)
+  return ignoredVars.includes(rootName)
+}
+
 module.exports = {
   meta: {
     type: 'problem',
@@ -139,6 +169,14 @@ module.exports = {
           ignoreLastCallback: {
             type: 'boolean',
           },
+          ignoreAssignmentVariable: {
+            type: 'array',
+            items: {
+              type: 'string',
+              pattern: '^[\\w$]+$',
+            },
+            uniqueItems: true,
+          },
         },
         additionalProperties: false,
       },
@@ -150,6 +188,10 @@ module.exports = {
   create(context) {
     const options = context.options[0] || {}
     const ignoreLastCallback = !!options.ignoreLastCallback
+    const ignoreAssignmentVariable = options.ignoreAssignmentVariable || [
+      'globalThis',
+    ]
+
     /**
      * @typedef {object} FuncInfo
      * @property {string[]} branchIDStack This is a stack representing the currently
@@ -244,6 +286,18 @@ module.exports = {
           return
         }
 
+        if (
+          ignoreAssignmentVariable.length &&
+          isLastCallback(node) &&
+          node.body?.type === 'BlockStatement'
+        ) {
+          for (const statement of node.body.body) {
+            if (isIgnoredAssignment(statement, ignoreAssignmentVariable)) {
+              return
+            }
+          }
+        }
+
         path.finalSegments.forEach((segment) => {
           const id = segment.id
           const branch = funcInfo.branchInfoMap[id]


Test patch:
diff --git a/__tests__/always-return.js b/__tests__/always-return.js
index cb888fd..7cc74de 100644
--- a/__tests__/always-return.js
+++ b/__tests__/always-return.js
@@ -105,6 +105,16 @@ ruleTester.run('always-return', rule, {
         .finally(() => console.error('end'))`,
       options: [{ ignoreLastCallback: true }],
     },
+    `hey.then(x => { globalThis = x })`,
+    `hey.then(x => { globalThis[a] = x })`,
+    `hey.then(x => { globalThis.a = x })`,
+    `hey.then(x => { globalThis.a.n = x })`,
+    `hey.then(x => { globalThis[12] = x })`,
+    `hey.then(x => { globalThis['12']["test"] = x })`,
+    {
+      code: `hey.then(x => { window['x'] = x })`,
+      options: [{ ignoreAssignmentVariable: ['globalThis', 'window'] }],
+    },
   ],
 
   invalid: [
@@ -230,5 +240,33 @@ ruleTester.run('always-return', rule, {
       options: [{ ignoreLastCallback: true }],
       errors: [{ message }],
     },
+    {
+      code: `hey.then(x => { invalid = x })`,
+      errors: [{ message }],
+    },
+    {
+      code: `hey.then(x => { invalid['x'] = x })`,
+      errors: [{ message }],
+    },
+    {
+      code: `hey.then(x => { windo[x] = x })`,
+      options: [{ ignoreAssignmentVariable: ['window'] }],
+      errors: [{ message }],
+    },
+    {
+      code: `hey.then(x => { windo['x'] = x })`,
+      options: [{ ignoreAssignmentVariable: ['window'] }],
+      errors: [{ message }],
+    },
+    {
+      code: `hey.then(x => { windows['x'] = x })`,
+      options: [{ ignoreAssignmentVariable: ['window'] }],
+      errors: [{ message }],
+    },
+    {
+      code: `hey.then(x => { x() })`,
+      options: [{ ignoreAssignmentVariable: ['window'] }],
+      errors: [{ message }],
+    },
   ],
 })