Open gurgunday opened 1 month ago
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.
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
just want to point out: it should be a breaking change to change a rule's default option.
Yeah I wish I'd opened the PR earlier to maybe target the v7 release
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, than making this change.
A different option could be allowing assignment to specific variables?
eg ignoreAssignmentVariable: ["global.window"]
Would this work for you?
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
@scagood, @brettz9 @aladdin-add, can you take a look?
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
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 100.00%. Comparing base (
bbcfcbf
) to head (c0179f4
). Report is 48 commits behind head on main.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
PTAL @brettz9 @scagood
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 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
PTAL with the current setting that only allows for simple var names
@brettz9 Do you want to take another look at this PR?
Sorry, is anyone else available? I've come into other work which is keeping me busy...
Adds
ignoreAssignmentVariable
as discussed in https://github.com/eslint-community/eslint-plugin-promise/pull/518#issuecomment-2260068048