feat(prefer-single-binding): add rule - #176
Conversation
✅ Deploy Preview for eslint-plugin ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
7edd618 to
15906fe
Compare
e12cac1 to
82797c1
Compare
82797c1 to
89651b2
Compare
25440ef to
c157176
Compare
kireevmp
left a comment
There was a problem hiding this comment.
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.
| schema: [ | ||
| { | ||
| type: "object", | ||
| properties: { | ||
| allowSeparateStoresAndEvents: { type: "boolean", default: false }, | ||
| enforceStoresAndEventsSeparation: { type: "boolean", default: false }, | ||
| }, | ||
| additionalProperties: false, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
Yes, you're right. I made a string flag now.
There was a problem hiding this comment.
You added three items here: forbid, allow and enforce like I suggested. Can you explain how allow is different during code generation from forbid and enforce 'separation"? And how would the user know clearly why allow works this way?
There was a problem hiding this comment.
The three modes differ only in what code generation is allowed to touch:
forbid— merges every call in the component into one, regardless of unit type.allow— merges calls that are entirely one type (two store calls → one store call). It never touches a mixed call and never moves a unit across types, so it removes same-type duplication only.enforce— does whatallowdoes and splits a mixed call into one call per type. End state: exactly one call per unit type.
So allow isn't "separation rules off" — it's "separating by type is your choice, duplicating within a type isn't". Internally allow and enforce share the same grouping pass; enforce just adds the mixed-call split on top.
On discoverability: the rule doc now leads the separation section with a table and spells the code-generation difference out before the per-mode examples, so the distinction is stated rather than inferred from samples.
| Value | Result | Forbids |
|---|---|---|
"forbid" |
exactly one useUnit call per component |
every extra call |
"allow" |
one call per unit type at most; a mixed call is left as-is | only same-type duplicate calls |
"enforce" |
exactly one call per unit type | same-type duplicates and mixed calls |
Leaving this open in case the naming still reads off to you — allow is the one I'm least attached to.
a7b21d9 to
2898b5b
Compare
kireevmp
left a comment
There was a problem hiding this comment.
Hey @Olovyannikov – thanks for taking the time to come back to polish. I see good progress here, I think there's a solid foundation now with normalization concept in place. What's left is to apply it consistently everywhere across the rule, to actually use that structure. The mental model here seems to be correct, which is great news.
If you're up to do another pass, I'd appreciate you also reviewing the test suite. Due to large rule size, under-employed normalization and ultimately duplicated code paths, the coverage is below 80% and the suite may not cover every corner case, something we really need here to be sure rule is sound.
| /** Unit expression nodes of a call, regardless of whether it can be normalized (used for hoisting analysis). */ | ||
| function unitExpressionNodes(call: UseUnitCall): Node.Expression[] { | ||
| const argument = call.init.arguments[0] | ||
| if (!argument || argument.type === NodeType.SpreadElement) return [] |
There was a problem hiding this comment.
Consider narrowing the selector on ESLint side to only include useUnit calls with Object, Array expressions or Identifier as first argument. May save couple of ifs across the rule.
There was a problem hiding this comment.
Partly done. The selector now pins arity and callee shape:
VariableDeclarator:has(> CallExpression.init[arguments.length=1][callee.type=Identifier])
I stopped short of pinning the argument type, though. Identifier isn't sufficient for the plain form — useUnit(CartModel.$cart) is a MemberExpression, and model-namespaced access like that is the common real-world shape. Covering it would mean :matches([arguments.0.type=ObjectExpression], [arguments.0.type=ArrayExpression], [arguments.0.type=Identifier], [arguments.0.type=MemberExpression], ...), an open-ended list that reads worse than the two checks it removes — and the argument still has to be type-checked in the visitor regardless, since that's what excludes @@unitShape.
Leaving this open: happy to add the :matches(...) narrowing if you'd still prefer it on the ESQuery side.
…inding Collapse the three parallel traversals (extractBindings, unitExpressionNodes, generateSeparationFix) into one analyze() pass that yields a single primitive: an identifier bound to the unit expression passed to useUnit. Every helper now consumes that structure, and source text is read only while building a fix. Resolve the soundness issues raised in review: - module units declared below the component are no longer a false TDZ hazard; only locals declared at/after the anchor block hoisting - multi-declarator statements, let/var and type-annotated bindings are reported without a suggestion instead of producing a crashing or invalid fix - the split fix keeps units whose type is undetermined instead of dropping them - duplicate destructuring keys no longer silently drop a binding - merging finds mergeable calls below one that cannot be hoisted - enforce now merges same-type calls as well as splitting mixed ones Drop the computed type aliases, the unsound `as Expression` cast and the redundant `as const`/type guards in favour of the built-in ts-eslint types. Tests cover the new edge cases; branch coverage rises from 79% to 89%.
…e cases Add a comparison of forbid/allow/enforce code generation, spell out which calls are ignored (@@unitShape, non-unit args, temporal dead zone) and which are reported without a suggestion (multi-declarator, let, type annotations, etc.).
prefer-single-binding stays available as effector/prefer-single-binding but is no longer bundled in a preset — removing a preset after release is painful, so hold off on adding one. Also order prefer-single-binding before prefer-useUnit.
The root `tsconfig.fixture.json` this suite pointed at was removed in effector#184, which moved every rule test onto `createRuleTester` and the on-disk fixture project in `src/testing/fixture`. Follow that migration.
Every other rule page carries one; without it the rule renders with an empty description in the docs rule table.
3b4f69d to
53347fb
Compare
…nction selectors ESLint strips a trailing `:exit` before parsing the selector, so a comma list carries it for every branch.
|
Rebased onto current Rebase. Picked up Review threads. I replied inline on all 25 and resolved the 21 that the normalization refactor closed out. Four are left open for you:
On the coverage concern from your last summary: the rule file is at 93.5% statements / 89.1% branches / 100% functions / 95.7% lines across 50 test cases, and it shrank from ~500 to 380 lines. Every soundness issue you listed in the "three distinct traversals" thread now has a named test. Current state: 399 tests pass, lint and |
Closes #116
Why is this important?
Performance
Each
useUnitcall creates its own subscription management overhead. Combining them reduces:Code clarity
A single
useUnitcall makes it easier to:Example
Separate stores and events calls
If you want to separate stores and events as independent groups, you can use option
separationWhen set to
allow, allows separateuseUnitcalls for stores and events, but still enforces combining multiple calls within each group.Example