feat(always-return): add ignoreAssignmentVariable option

[gurgunday]

Adds ignoreAssignmentVariable as discussed in #518 (comment)

[github-actions]

PR Compliance Checks Passed!

[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. 🤔

In fact I would prefer to remove the rule from the recommended config, than 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, @brettz9 @aladdin-add, can you take a look?

[brettz9]

Besides the one suggestion, LGTM...

[scagood]

I generally prefer this strategy I think!

[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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (bbcfcbf) to head (064637a).
Report is 63 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #518   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        26    +1     
  Lines          649       719   +70     
  Branches       250       278   +28     
=========================================
+ Hits           649       719   +70     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[gurgunday]

PTAL @brettz9 @scagood

[scagood]

I would love to see some more tests, other than that, I think this is starting to take shape 💪

[brettz9]

LGTM...

Just as an idea for a future PR, it would seem to me a possibly even more common use browser case to allow node appending in the last promise, e.g.:

prom.then((newInfo) => {
  document.body.append(toDom(newInfo));
})

But regardless, this PR LGTM...

[gurgunday]

Okay, so #518 (review) made a good point - we need to take into account both variations (computed or not computed) for each level of the passed variable names

For instance, if ignoreAssignmentVar: ["window.a.b"] is passed, assignments to window.a.b window.['a']['b'] and window.a['b'] should all pass; and the passed variable name itself can have variations as well like ignoreAssignmentVar: ["window.['a']['b']", "window.a.b"]

It's not hard but it requires a little more complex logic or at least we need to determine how the config should behave

To keep the implementation correct, what I propose is that we start with 1 level of nesting in the config - e.g., just window or globalThis - and after discussing the details we make a follow-up PR to lift this restriction

[gurgunday]

PTAL with the current setting that only allows for simple var names

[scagood]

To keep the implementation correct, what I propose is that we start with 1 level of nesting in the config - e.g., just window or globalThis - and after discussing the details we make a follow-up PR to lift this restriction

I agree, I have left comments inline with that 😄

[scagood]

I like the shape of this now!
Thank you for the awesome docs too 😀

[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...

[gurgunday]

Hey everyone, just checking in to see if we can get this merged soon

Doing a lot of webdev these days and I keep bumping into this 🤣