Repository navigation
lint(no-raw-private-attribute): keep private data attributes in one owner module, and enforce with a baseline - #9214
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
PR Analysis Report🔎 Preview ·
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested: the rule, its public list and the 92-site baseline match INV23, and the baseline check and plugin tests pass, but two small fixes are needed first.
- The per-file baseline allowance in
no-raw-private-attribute.js(lines 230-249) is a copy of the one inno-nullish-jsx-guard.js; move it into one shared helper that both rules call. - The owner-module export check misses
export default 'data-astryx-press'and selectors exported inside an object, an array or a function's return value, so either report those or narrow the README and rule header to the shape it catches.
INV23's known-gaps note still says the rule does not exist yet and is worth updating in the same push.
[Automated review]
| const options = context.options[0] ?? {}; | ||
| const root = options.root ?? context.cwd; | ||
| const relative = path | ||
| .relative(root, context.filename ?? context.getFilename()) | ||
| .split(path.sep) | ||
| .join('/'); | ||
| // Each listed attribute allows as many sites as it is listed; the next | ||
| // one is reported. | ||
| const allowance = new Map(); | ||
| for (const attribute of options.baseline?.[relative] ?? []) { | ||
| allowance.set(attribute, (allowance.get(attribute) ?? 0) + 1); | ||
| } | ||
| function isAllowed(attribute) { | ||
| const left = allowance.get(attribute) ?? 0; | ||
| if (left === 0) { | ||
| return false; | ||
| } | ||
| allowance.set(attribute, left - 1); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
This block (options, repo-relative file, per-entry count map, isAllowed) is a line-for-line copy of no-nullish-jsx-guard.js lines 386-402. Could it move into one exported helper, in shared.js or beside runRuleBaseline, that both rules call, so a fix to baseline matching reaches both?
| function noteOwnerExport(node, names) { | ||
| if (ownedName == null || !names.includes(ownedName)) { | ||
| return; | ||
| } | ||
| let holder = node.parent; | ||
| while (unwrap(holder) !== holder) { | ||
| holder = holder.parent; | ||
| } | ||
| if ( | ||
| holder?.type !== 'VariableDeclarator' || | ||
| unwrap(holder.init) !== node | ||
| ) { | ||
| return; | ||
| } | ||
| ownerExports.push({node, declarator: holder}); | ||
| } |
There was a problem hiding this comment.
This only catches a string that is the whole initializer of an exported const. In an owner module, export default 'data-astryx-press', export const selectors = {pressed: '[data-astryx-press="on"]'}, an exported array, or an exported function returning the name all pass, while the README says the module may not export the raw name or a selector as a string. Either cover those shapes (object keys can stay allowed for setter props) with tests, or narrow the README and header wording.
There was a problem hiding this comment.
Changes requested: the rule, its public list and the 92-site baseline match INV23, both earlier points are addressed, and the plugin tests and baseline check pass. One gap remains: in an owner module, a selector built from the name constant in a template (export const PRESSED_SELECTOR = `[${ATTRIBUTE}="on"]`) is not reported, although the README and rule header say an export through a module constant is. Either let valueDestination follow a value out of a template slot (and a + operand) with RuleTester cases, or name that shape in Known limitations, the README and the INV23 gap note.
[Automated review]
| * decide, or `null` when it is consumed some other way (an object key, a call | ||
| * argument, a member access). | ||
| */ | ||
| function valueDestination(node) { |
There was a problem hiding this comment.
A value that lands in a template's ${} slot stops here as consumed, so in an owner module const ATTRIBUTE = 'data-astryx-press'; export const PRESSED_SELECTOR = `[${ATTRIBUTE}="on"]`; (or an exported function returning that template) passes with no report, while the README and header say an export through a module constant is reported. Could a TemplateLiteral expression slot and a + operand pass the value through, with RuleTester cases, or the shape go into Known limitations?
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
Approval recommended: the rule, its public list and the 92-site baseline match INV23, the owner-module export check now follows a value through a template slot and a + operand with tests, and the plugin tests, baseline check and lint pass. Two optional cleanups, noted inline: drop the .doc.mjs example from the rule header, since the repo config does not lint .mjs outside the CLI, and add templates that reach the namespace through anything but dataAttrNamespace or NAMESPACE to Known limitations.
[Automated review]
| * Reported, outside the owner modules: | ||
| * - a string literal that contains `data-astryx-<name>`, in any position | ||
| * (an object key, a `setAttribute` argument, a selector inside | ||
| * `stylex.create` or `stylex.when.*`, a `.doc.mjs` description); |
There was a problem hiding this comment.
Optional: the repo config ignores **/*.mjs outside the CLI (eslint.config.js:200), so a .doc.mjs description is never linted in practice, as the README and the INV23 gap note say. Dropping the .doc.mjs example here keeps the header in line with them.
| * Public, never reported: the names INV23 lists (`PUBLIC_ATTRIBUTES`). | ||
| * `naming.ts`, which builds every `data-astryx-*` name, is not checked. | ||
| * | ||
| * Known limitations: a name assembled with `+` or read through |
There was a problem hiding this comment.
Optional: a template that reaches the namespace through anything but dataAttrNamespace or NAMESPACE (data-${naming.dataAttrNamespace}-press, an aliased import, data-${classPrefix}-press) is neither read nor reported as data-astryx-*. No shipped source does this today; naming it here would complete the list.
…very export route, update the INV23 gap note
…es and string concatenation
…his lint-only change
ae0a756 to
e7e6636
Compare
There was a problem hiding this comment.
Changes requested: the rule, its eight-name public list and the 92-site baseline match INV23, and the plugin tests, baseline check and strict lint pass, but the owner-module match rejects the module current spec already names. architecture:interaction-modality INV3 puts the focus-owner attribute in privateAttributes/focusOwner.stylex.ts, while the rule compares that camelCase file stem with the lowercased attribute name (no-raw-private-attribute.js:308, :326), so the module fails strict lint for its own data-astryx-focus-owner (9 errors on the version in #8869). Converting the stem to kebab case before comparing, naming owner paths in camelCase in the messages and README, and adding RuleTester cases for that path would fix it.
[Automated review]
| if (NAMING_MODULE.test(filename)) { | ||
| return {}; | ||
| } | ||
| const ownedName = OWNER_MODULE.exec(filename)?.[1] ?? null; |
There was a problem hiding this comment.
The owned name is the file stem as written, but every attribute name is lowercased before the comparison at line 326, so a camelCase owner never matches. architecture:interaction-modality INV3 names privateAttributes/focusOwner.stylex.ts as the focus-owner owner, and that module gets foreignOwner for its own data-astryx-focus-owner (9 errors on the version in #8869). Converting the stem to kebab case here (focusOwner → focus-owner), with a RuleTester case for that path, would fix it.
…r kebab-case attribute
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Approval recommended: the rule, its eight public names and the 92-site baseline match INV23, the owner-module match now accepts privateAttributes/focusOwner.stylex.ts with tests, and the plugin tests and baseline check pass. Two optional follow-ups: the description's note that #8869's focusOwner.stylex.ts lints clean holds only for its own attribute, since it still gets 5 reports for data-astryx-focus-owner-group. Comparing camelCase(name) with the file stem, instead of kebab-casing the stem, would let owner paths round-trip for digit-led names such as data-astryx-col-2 and stop accepting a kebab-case file name beside the camelCase one.
[Automated review]
| // Owner modules are named in camelCase (`focusOwner.stylex.ts` owns | ||
| // `data-astryx-focus-owner`); compare in the attribute's kebab case. | ||
| const ownerStem = OWNER_MODULE.exec(filename)?.[1] ?? null; | ||
| const ownedName = ownerStem == null ? null : kebabCase(ownerStem); |
There was a problem hiding this comment.
Optional: kebabCase cannot restore the hyphen camelCase drops before a digit, so for data-astryx-col-2 the message names col2.stylex.ts and a module there is then reported for its own attribute; a kebab-case focus-owner.stylex.ts is also accepted beside focusOwner.stylex.ts. Comparing camelCase(name) with ownerStem (here and in noteOwnerValue) fixes both. No attribute has a digit-led segment today.
Maintainer impact
A contributor who needs a private styling hook writes
data-astryx-<name>by hand: as a JSX attribute, asetAttributecall, or a selector insidestylex.create. Nothing stops it, so the same private attribute ends up spelled in many files (data-astryx-pressalone is written at 45 sites across 14+ files). Renaming or reshaping one means hunting every copy, and a private attribute can drift into a public export or a doc without anyone noticing. Consumers are unaffected either way; this is about keeping Astryx's own source maintainable.Intended invariant
architecture:public-component-apiINV23 (current): each privatedata-astryx-*attribute has one owner module in the package that writes it,packages/<package>/src/privateAttributes/<name>.stylex.ts, that writes its name and selectors once and exports ready-made styles, setter props and readers. Outside the owner modules and tests, a hand-typeddata-astryx-string, template, or JSX attribute name, or adataAttr()call that names a private attribute, fails@astryx/no-raw-private-attribute; sites that predate the rule sit in its baseline, which only shrinks.Change
New rule
@astryx/no-raw-private-attributein the internal ESLint plugin. Outside owner modules it reports:data-astryx-<name>in any position (object key, call argument, selector instylex.create/stylex.when.*);data-${dataAttrNamespace}-…);data-astryx-<name>;dataAttr('<name>')call naming a private attribute;data-astryx-${part},dataAttr(name)) asdata-astryx-*.Owner modules are named in camelCase (
focusOwner.stylex.tsownsdata-astryx-focus-owner, asarchitecture:interaction-modalityINV3 names it). Inside an owner module, only its own attribute may be written, and the name or a selector may not leave the module as a string: not as an exported or default-exported value, inside an exported object, array or function's return value, or through a module constant one of those uses, including a selector built from that constant in a template or with+. It stays allowed as an object key (setter props,stylex.createselectors) and a call argument (writers,closest/matchesreaders). Importing another package's owner module is reported.Public allowlist (never reported), exactly the list INV23 names:
data-astryx-theme,data-astryx-media,data-astryx-aspect-ratio-override,data-astryx-token,data-astryx-token-value,data-astryx-menu-press,data-astryx-pressable,data-astryx-edge-comp. A longer name that merely starts with a public one (data-astryx-theme-prose) is private.naming.ts, which builds every name, is not checked.Enforcement, the same shape as
no-internal-barrel-importandno-nullish-jsx-guard:eslint.config.jsturns it on forpackages/*/src(tests, stories and test helpers excluded, since INV23 lets tests name private attributes) at the strict-mode severity, with{baseline, root}.no-raw-private-attribute.baseline.jsonlists the 92 sites in 41 files that predate the rule, once per site: core 90, lab 1, richtext 1. Biggest entries:data-astryx-press45,data-astryx-popover-fallback-close5,data-astryx-cell-gliding4,data-astryx-id4.no-raw-private-attribute.baseline.mjsrewrites the baseline;--checkverifies it.Shared baseline matching:
baselineAllowance()inrule-baseline.mjsis the one per-file, counted allowance; this rule andno-nullish-jsx-guardboth call it.README section for the rule. The rule is registered in the plugin but not added to the shared presets, like the barrel rule.
No source is migrated here; moving each attribute into its owner module is follow-up work, and each move shrinks the baseline.
Risk
None at runtime: lint-only, no package source or published output changes. The only effect is that a pull request adding a new hand-written private attribute fails lint in CI.
CI impact
Evidence
el.setAttribute('data-astryx-foo', '')in any shipped source passes lint in every mode.data-astryx-foois a private data attribute…"). Strict lint overpackages/*/srcreports no errors with the baseline, and--checkpasses.Scope
Testing
internal/eslint-plugin-astryx/no-raw-private-attribute.test.mjs(RuleTester):dataAttrNamespace), JSX attribute, object key, selector insidestylex.createandstylex.when.ancestor, two attributes in one selector,data-astryx-theme-prose,dataAttr('id'),dataAttr(name), interpolated name and bare prefix, a Lab site, one site beyond the baseline count, a baseline for another file or attribute, an owner module writing another attribute or exporting its name or a selector, cross-package owner imports;+, and viadataAttr();naming.ts, and baselined sites.no-nullish-jsx-guardtests on the shared allowance).no-raw-private-attribute.baseline.mjs --checkpasses; it lints everypackages/*/srcfile in scope with the repo config.document.body.setAttribute('data-astryx-foo', '')added to Avatar fails strict lint with the rule's message, then removed.privateAttributes/focusOwner.stylex.ts) writes its owndata-astryx-focus-ownerwithout a report; a second attribute in that file (data-astryx-focus-owner-group) is reported, as one owner per attribute requires.