feat(prefer-single-binding): add rule

[Olovyannikov]

Closes #116

Why is this important?

Performance

Each useUnit call creates its own subscription management overhead. Combining them reduces:

• Number of hook calls
• Subscription management overhead
• Re-render coordination complexity

Code clarity

A single useUnit call makes it easier to:

• See all dependencies at a glance
• Understand component's reactive logic
• Maintain and refactor code

Example

// correct
const Component = () => {
  const [value, setValue] = useUnit([$store, storeUpdated]);
}

// incorrect
const Component = () => {
  const [value] = useUnit([$store]);
  const [setValue] = useUnit([storeUpdated]);
}

Separate stores and events calls

If you want to separate stores and events as independent groups, you can use option separation

When set to allow, allows separate useUnit calls for stores and events, but still enforces combining multiple calls within each group.

Example

// correct
const Component = () => {
  const [value, setValue] = useUnit([$store, storeUpdated]);
}

// if `separation` flag is 'enforce':
const Component = () => {
  const [value, setValue] = useUnit([$store, storeUpdated]);
  
   // will be transformed to:
   
  const [value] = useUnit([$store]);
  const [setValue] = useUnit([storeUpdated])
}

// also correct
const Component = () => {
  const [value] = useUnit([$store]);
  const [setValue] = useUnit([storeUpdated]);
}

// incorrect
const Component = () => {
  const [value] = useUnit([$store]);
  const [setValue] = useUnit([storeUpdated]);
  const [setAnotherValue] = useUnit([thirdEvent]);
}

[netlify]

✅ Deploy Preview for eslint-plugin ready!

Name	Link
🔨 Latest commit	2898b5b
🔍 Latest deploy log	https://app.netlify.com/projects/eslint-plugin/deploys/69e1eb1a2e67780008c3c9dd
😎 Deploy Preview	https://deploy-preview-176--eslint-plugin.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[Olovyannikov]

@kireevmp can you review this code please?
I did it similarly to #175. I removed the unit type detection using heuristics and replaced it with type-check. And updated the test cases at the same time.

[kireevmp]

Hey @Olovyannikov! I skimmed through the PR and before we dive into actual implementation specifics, let's work out what this rule should check for and what we'd consider a violation. Right now it seems there are a few edge cases that need considering.

[kireevmp]

allow and enforce settings are not actually independent, because you can't enforce separation while also disallowing it. These are better merged into a string flag, i.e. separation: "forbid" | "allow" | "enforce"

[Olovyannikov]

Yes, you're right. I made a string flag now.

[kireevmp]

The code fix output is not runtime equivalent to the input, so we shouldn't use autofix here. Since it modifies the runtime behavior, it must be a suggestion.

[kireevmp]

The plugin generally requires TypeScript, and eslint-typescript guidelines recommend against rules that change behavior based on type information availability. So I suggest directly getting services as you need where you need without try/catch handling and other conditionals.

[kireevmp]

Consider merging selectors into a single "logical" one FunctionDeclaration, FunctionExpression, ArrowFunctionExpression

[Olovyannikov]

Indeed, you can simply list them separated by commas. Did it, thanks.

[kireevmp]

1. This is a react rule, not recommended one – it only concerns React users and not Vue users, for example
2. Since this is a stylistic rule (enforces code style, not correctness), we can't really include it into react preset either

[Olovyannikov]

How about the style group?

[kireevmp]

That'd be something close to react-stylistic preset, yeah. But I'm not sure we want to make a new preset just for one rule. Let's ponder on this once implementation is close to completion.

[kireevmp]

I'm a bit concerned with rule expectation mismatch here. It declares that it "recommends a single useUnit call", however

1. This rule does work with about non-destructuring calls const a = useUnit($a) and non-combining calls const { a, b } = useUnit(model) with property access
2. There are no tests illustrating how this rule works with Effects
3. There are no tests that combines both array form and object form in a single component
4. There are no tests that play with variable positioning, for example where the unit consumed is not defined statically

   const { a } = useUnit({ a: $a })
   const { $b } = useContext(MyModel)
   const { b } = useUnit({ b: $b })

5. This rule does not declare how it works with @@unitShape (i.e. routers, queries)
6. With a proper setting, it actually enforces 2 useUnit calls with separation, so not a "single" call
7. I've also not found tests demonstrating how this rule works with aliasing

   const { a } = useUnit({ a: $a });
   const { a: b } = useUnit({ a: $b })

My question here is – what are the expectations in these cases? Is there a sound "fix" we can provide for all of them? And should we actually check and report those cases?

[Olovyannikov]

Yes, I will add more cases.

[kireevmp]

Can we take a step back and work on this in comments first? It would be much easier to discuss conceptual solutions without diving into code other than examples. I'd love your opinions on what solutions, if any, are available for every listed case

[Olovyannikov]

Aliasing works - I've added tests for this.
Also, we can't offer a fix for all cases; for such cases, we'll simply mark it as a warning.
Regarding effects, there's an issue suggesting creating a rule that prohibits working with effects directly. If that rule is included in the recommended rule, you can omit that case here, but by default, I currently separate calls when using separation enforce.

What about UnitShape - I'm not sure what the correct approach would be - highlight it as a warning? Or skip it, since it operates according to special rules?

Examples:

separation: 'forbid'

// Store + Event
// ESLint: Multiple useUnit calls detected. Consider combining them into a single call for better performance. (effector/prefer-single-binding)

const products = useUnit(CartModel.$cart);
const onProductRemoveFromCart = useUnit(AddToCartModel.cartProductTotalRemoved);

// Store + Event Array Shaped
// ESLint: Multiple useUnit calls detected. Consider combining them into a single call for better performance. (effector/prefer-single-binding)
// Suggest to merge this calls included

const [products] = useUnit([CartModel.$cartProducts]);
const [onProductRemoveFromCart] = useUnit([AddToCartModel.cartProductTotalRemoved]);

// Store + Event Object Shaped
// ESLint: Multiple useUnit calls detected. Consider combining them into a single call for better performance. (effector/prefer-single-binding)
// Suggest to merge this calls included

const { products } = useUnit({
        products: CartModel.$cartProducts,
});
const { onProductRemoveFromCart } = useUnit({
      onProductRemoveFromCart: AddToCartModel.cartProductTotalRemoved,
});

// Mixed shapes - Array + Object
// ESLint: Multiple useUnit calls detected. Consider combining them into a single call for better performance. (effector/prefer-single-binding)

const [products] = useUnit([CartModel.$cartProducts]);
const { onProductRemoveFromCart } = useUnit({
  onProductRemoveFromCart: AddToCartModel.cartProductTotalRemoved,
});

// Using alias
// ESLint: Multiple useUnit calls detected. Consider combining them into a single call for better performance. (effector/prefer-single-binding)

const { products } = useUnitEffector({
        products: CartModel.$cartProducts,
    });
const [onProductRemoveFromCart] = useUnitEffector([AddToCartModel.cartProductTotalRemoved]);

// named destructuring
// ESLint: Multiple useUnit calls detected. Consider combining them into a single call for better performance. (effector/prefer-single-binding)

const { a } = useUnitEffector({
        a: CartModel.$cartProducts,
    });
const { a: b } = useUnitEffector({
    a: AddToCartModel.cartProductTotalRemoved,
  });

[kireevmp]

With both @@unitShape and a pre-build useUnit(model) where the argument is not a unit or shape of units, I'd suggest we fully exclude them from calculation. These can never be soundly "combined" so they're logically not subject to rule enforcement / separation.

---

With Store + Event basic case, this is actually the most common example of "invalid" code when developers duplicate useUnit calls without using destructuring, common copy-pasting. So we ought to provide a suggestion for this simplest case.

---

With mixed shapes - Array + Object case we can also provide a suggestion by treating

const [products] = useUnit([CartModel.$cartProducts]);
// as equivalent to
const { products } = useUnit({ products: CartModel.$cartProducts });

and merging with other destructuring patterns.

This case will always provide a suggestion unless either a rest property is used or variable names are duplicated in scope.

Using aliases adds a layer of complexity, however the accepting identifier is bound to be unique due to scoping rules, so

const { a: b } = useUnitEffector({ a: CartModel.$cartProducts });
// can be treated as
const { b } = useUnitEffector({ b: CartModel.$cartProducts });

for the sake of suggestion generation.

---

So the most notably here are the cases we aren't able to provide a suggestion:

1. @@unitShape and useUnit(model) – ignored by the rule due to unsound combination,
2. duplicate identifiers to prevent invalid JS from being generated, or
3. rest property and spread operator since we can't track property -> unit mapping.

Other cases should theoretically provide a valid suggestion.

[kireevmp]

There's also a useUnit ordering problem since it can be applied to dynamically acquired Units, see example above with useContext. So a question is how would we treat the following code?

const a = useUnit($a) // (1)
const $b = useContext(MyModel) // (2)
const b = useUnit($b) // (3)

Should those be kept separate, suppressing the warning, or do we generate a suggestion for

const $b = useContext(MyModel)
const { a, b } = useUnit({ a: $a, b: $b })

And if we do so, how do we track if a has been used between (1) and (3)?

It seems even for basic cases this rule would also unfortunately require variable scope analysis to detect if units are declared in a current scope or a surrounding scope to prevent issues with temporal dead zone.

[kireevmp]

Let's use a shared typecheck helper is in this rule instead of custom checks?

[Olovyannikov]

Cool! Found it, using it! :)

[kireevmp]

I'm not sure why we need to score something here and pre-group? Would it be possible to just collect the list of mappings unit -> local ident that is grouped later, with no regard of who was first or second, completely regenerating all useUnit calls?

[Olovyannikov]

I agree, it looks unreliable. I'll see what I can do.