Repository navigation
spec(public-component-api): INV23's gap note names the lint that now runs - #9237
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
/approve-spec 0ed6dc1 |
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
The INV23 gap note's update saying the lint now runs is accurate, but two wording fixes are needed before approval. The new clause "each baselined site conforms by moving into its owner module" does not hold for the baselined Lab Schedule site, which reads Core's data-astryx-popover-fallback-close: INV23 forbids importing another package's owner module, and the bullet's last sentence already gives that site a different path, so drop the clause or point to the paths below. Removing "public-API review is the check" also leaves the rule's own blind spots unnamed (Changesets, and names built with + or read through element.dataset), so keep a short clause saying public-API review covers them.
[Automated review]
| the sites that predate it, and no owner module exists yet; each baselined | ||
| site conforms by moving into its owner module. The `data-astryx-press` selector is copied in 14 Core files. |
There was a problem hiding this comment.
This clause doesn't hold for every baselined site. packages/lab/src/Schedule/useScheduleViewPopover.ts is in the baseline and reads Core's data-astryx-popover-fallback-close. INV23 forbids importing another package's owner module, and the last sentence of this bullet already says that site conforms through Core's public API or an attribute Lab owns. Consider dropping the clause, or writing "each baselined site conforms as described below".
PR Analysis Report🔎 Preview ·
|
…r the rule's blind spots
|
/approve-spec 61c6629 |
There was a problem hiding this comment.
The lint-status update is accurate and the earlier conformance clause is gone, but one wording fix is still needed. INV23 bars private attributes from .doc.mjs files, and the ESLint config ignores every .mjs file outside packages/cli, so the rule never reaches component docs; ClickableCard, SelectableCard and Thumbnail docs name data-astryx-press today. Please list .doc.mjs files beside Changesets in the new clause. While there, match the rule's own limitations note: 'data-astryx-' + name is already reported, while names assembled from the namespace with +, or reached through a member access or aliased import in a template, are not.
[Automated review]
| the sites that predate it, and no owner module exists yet; public-API review | ||
| covers what the rule cannot see (Changesets, and names built with `+` or read | ||
| through `element.dataset`). The `data-astryx-press` selector is copied in 14 Core files. |
There was a problem hiding this comment.
This list of what the rule cannot see leaves out .doc.mjs files. INV23 bars private attributes from them, but eslint.config.js ignores **/*.mjs outside packages/cli, so the rule never lints them. A data-astryx-press selector that fails lint in a .ts file is skipped in a .doc.mjs one, and ClickableCard.doc.mjs, SelectableCard.doc.mjs and Thumbnail.doc.mjs name data-astryx-press today. Suggested wording:
public-API review covers what the rule cannot see: Changesets and `.doc.mjs`
files, which lint does not reach, and names assembled from the namespace with
`+`, built in a template through a member access or aliased import, or read
through `element.dataset`.
…pressions the lint cannot see
|
/approve-spec 90961ca |
There was a problem hiding this comment.
The lint-status update and the new .doc.mjs item are accurate, but one wording fix is still needed. The rewritten list of what the rule cannot see drops element.dataset, which the rule's own limitations note (no-raw-private-attribute.js:42-47) names and which lint does not report, whether read or written; it also leaves out templates built from classPrefix. Please add both, for example: "names assembled from the namespace with + or reached in a template through anything but dataAttrNamespace or NAMESPACE (a member access, an aliased import, classPrefix), and names read or written through element.dataset."
[Automated review]
| covers what the rule cannot see: Changesets, component `.doc.mjs` files (the | ||
| lint does not reach them), and names assembled from the namespace with `+` or | ||
| reached through a member access or aliased import in a template. The `data-astryx-press` selector is copied in 14 Core files. |
There was a problem hiding this comment.
This list now reads as complete, but it drops element.dataset, which the previous wording named. The rule's limitations note (internal/eslint-plugin-astryx/no-raw-private-attribute.js:42-47) says the lint does not see a name read through element.dataset, nor a template that reaches the namespace through any identifier other than dataAttrNamespace or NAMESPACE (it names classPrefix), and that public-API review covers them; el.dataset.astryxPress = 'on' and `data-${classPrefix}-press` both pass strict lint on this head. Suggested wording:
names assembled from the namespace with `+` or reached in a template through
anything but `dataAttrNamespace` or `NAMESPACE` (a member access, an aliased
import, `classPrefix`), and names read or written through `element.dataset`.
|
/approve-spec 87cd067 |
There was a problem hiding this comment.
The INV23 gap note now matches what the @astryx/no-raw-private-attribute lint does, and this looks good to merge once required checks pass. The rule runs with its baseline, every form the note hands to public-API review (Changesets, .doc.mjs files, + and non-namespace template names, element.dataset reads and writes) passes strict lint on this head, and the earlier element.dataset and classPrefix omission is fixed. One optional follow-up outside this pull request: a dataAttr call through an aliased import (import {dataAttr as attr}) also slips past the rule, so its own limitations note could name it.
[Automated review]
Maintainer impact
The private-attribute lint now runs, so the INV23 known-gaps note no longer says it doesn't exist. Factual update only; no rule changes.
Decision
Consistency edit: INV23's known-gaps bullet now says the
@astryx/no-raw-private-attributerule runs with a baseline of the sites that predate it, and that no owner module exists yet.Writing rubric results