Repository navigation
chore(lint): Core's internal subpaths stay inside Core and Lab - #8561
Conversation
Core is about to export a few undocumented modules under `@astryxdesign/core/internal/<name>`, so Lab can build on machinery with no public API of its own (the reorder engine behind createReorderable first). They are reserved for Astryx's own packages and sit outside the compatibility promise (spec:AST-017 FR21). A new rule, @astryx/no-core-internal-import, reports any import, re-export or dynamic import of `@astryxdesign/core/internal` or a path under it. The repo config turns it on for every file outside packages/core and packages/lab. It is a custom rule rather than a repo-wide `no-restricted-imports` block, because flat config overrides a same-named rule per file and would undo the layer blocks that already use it. The plugin README records the convention each internal subpath follows: an explicit `./internal/<name>` exports entry, never a wildcard, and a header saying it is reserved, undocumented and outside the promise. Lint-only; nothing ships, so no changeset.
The previous commit's formatter pass reflowed three unrelated selectors.
The previous commit emptied the file by mistake. This puts back main's config with only the internal-subpath block added.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
PR Analysis Report
|
meta-cla
Bot
added
the
CLA Signed
…rule Core's internal subpaths share one convention (meta#8561): an explicit ./internal/<name> exports entry, a header saying it is reserved for Astryx's own packages, undocumented and outside the compatibility promise (spec:AST-017 FR21), and the custom @astryx/no-core-internal-import rule for files outside Core and Lab. This drops the touch module's own repo-wide no-restricted-imports block, which flat config would let override the layer blocks that already use that rule, and points the entry's header and the lint test at the shared rule. Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
The rule behaves as described for ordinary imports, but approval needs an engineering owner to settle the governing spec for the ./internal/* subpaths it protects: on main, spec:AST-017 FR21 says behavior modules without a public contract are never exported and architecture:public-component-api INV12 treats anything reachable from a package path as public, while the carve-out the README and lint message cite exists only in the open amendment #8464. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so if they are an engineering owner); if it is approved, the README's FR21 citation holds once #8464 is on main, and if it is rejected, the README, rule header and lint message should stop describing internal subpaths as exported and outside the compatibility promise. Separately, template-literal import() and typeof import() references pass the rule and .mjs files outside packages/cli are never linted, so please cover those two forms in the rule and its tests and narrow the README's coverage sentence.
[Automated review]
| check(node.source, node.source.value); | ||
| }, | ||
| ImportExpression(node) { | ||
| if (node.source.type === 'Literal') { |
There was a problem hiding this comment.
Only a Literal source is checked here, so import(`@astryxdesign/core/internal/reorder`) passes, and the rule has no visitor for typeof import('@astryxdesign/core/internal/reorder') type references. Accepting a TemplateLiteral with no expressions and adding a TSImportType visitor, each with a test case, closes both.
| Astryx's own packages and sit outside the compatibility promise | ||
| (`spec:AST-017` FR21). The repo config turns this rule on for every file outside | ||
| `packages/core` and `packages/lab`, so an import of an internal subpath from an | ||
| app, a template or another package is an error. |
There was a problem hiding this comment.
On main, spec:AST-017 FR21 says a behavior or utility module without a public contract "is never exported, as experimental or otherwise"; the ./internal/* carve-out cited here is the amendment proposed in #8464, so this citation holds only once that lands. Also, .mjs files outside packages/cli are never linted (the global '**/*.mjs' ignore in eslint.config.js), so "every file outside packages/core and packages/lab" overstates the coverage.
…ort(); README settles the layout
From review: the rule missed `require('@astryxdesign/core/internal/…')`
(the config applies it to .cjs) and TypeScript's
`typeof import('@astryxdesign/core/internal/…')`; both are reported now,
with a test each, and the tests parse with typescript-eslint.
The README describes one layout: an internal subpath is a single file,
`packages/core/src/internal/<name>.ts`, with an explicit
`./internal/<name>` exports entry listed in scripts/sync-exports.js's
STATIC_EXPORTS, and no longer names a module that does not exist yet.
…ule subpaths, as internal/touch is
…on and touch-support imports
The gesture core, motion and touch support have no entry of their own
(architecture:react-component-runtime INV13). Outside Core and Lab the rule
now also reports a deep import of them, by package path
(`@astryxdesign/core/gestures`, `@astryxdesign/core/motion/*`) or by a path
into Core's source (`…/core/src/{gestures,motion,touch-support}/…`), through
every form it already checks (import, export from, import(), require(),
typeof import()), with a test for each and for look-alike names. One rule
does this job, rather than a second `no-restricted-imports` block.
There was a problem hiding this comment.
The rule works for ordinary imports, but approval waits on an engineering owner settling the spec for the ./internal/* subpaths it protects: on main, spec:AST-017 FR21 says behavior modules without a public contract are never exported and architecture:public-component-api INV12 treats anything reachable from a package path as public, while the carve-out and the architecture:react-component-runtime INV13 that the README and lint messages cite exist only in the open amendment #8464. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so if they are an engineering owner); otherwise the citations hold once #8464 is on main, and if the carve-out is rejected the README, rule header and lint messages should stop describing internal subpaths as exported and outside the compatibility promise. Separately, please make the rule report a template-literal import() specifier, note in the README that .mjs files outside packages/cli are not linted, and drop the internal/touch precedent, which is not on main (it arrives with #8465).
[Automated review]
| check(node.source, node.source.value); | ||
| }, | ||
| ImportExpression(node) { | ||
| checkLiteral(node.source); |
There was a problem hiding this comment.
import(`@astryxdesign/core/internal/reorder`) passes lint with no diagnostic: checkLiteral accepts only a Literal, so a template literal with no expressions is never checked, although the README says every import() is reported. Accepting a TemplateLiteral whose expressions is empty (its one quasi's value.cooked), with an invalid test case, closes it.
| `scripts/sync-exports.js` (`FILE_MODULE_SUBPATH_EXPORTS`), as `internal/touch` | ||
| is. The rule reports every way to reach one: |
There was a problem hiding this comment.
internal/touch is not in FILE_MODULE_SUBPATH_EXPORTS (scripts/sync-exports.js:225-237) or in Core's exports, on this branch or on main; #8465 adds it. Dropping "as internal/touch is", or naming it as the planned first entry, keeps the README true if this lands first.
…ly what the rule reports They list the fenced paths and forms and that only Core and Lab may import them, without naming internal entries that do not exist yet.
`@astryx/no-core-internal-import` (meta#8561) already reports `@astryxdesign/core/internal/*`, the gesture and motion package paths and the core/src gesture, motion and touch-support paths outside Core and Lab, in TypeScript and JavaScript files alike. This entry's separate `no-restricted-imports` block goes, so one rule does the job and no same-named block can replace another under flat config. The entries' lint test reads that rule. Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
The global ignores still leave some scripts unlinted, so the header and README say "every linted file" rather than every file.
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision: on main, spec:AST-017 FR21 says behavior modules without a public contract are never exported and architecture:public-component-api INV12 treats anything reachable from a package path as public, while the ./internal/* carve-out, Lab's access to Core internals and the architecture:react-component-runtime INV13 that the README and lint messages cite exist only in the open amendment #8464. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so if they are an engineering owner), or the citations hold once #8464 is on main; either way, the decision should say whether Lab keeps direct access to Core's gesture, motion and touch-support source, since #8464's INV13 routes Lab through @astryxdesign/core/internal/touch. Separately, a template-literal import() and a relative path into packages/core/src/internal/ pass the rule although the README says only Core and Lab can import an internal subpath, so please report both forms and add a test for each.
[Automated review]
| messages: { | ||
| internal: | ||
| "'{{source}}' is one of Core's internal subpaths, reserved for Astryx's own packages (Core and Lab) and outside the compatibility promise. Use Core's public API instead.", | ||
| deep: "'{{source}}' reaches into Core's gesture, motion or touch-support source, which has no entry of its own and is for Core and Lab only (architecture:react-component-runtime INV13). Use Core's public API instead.", |
There was a problem hiding this comment.
This message, with the packages/lab/** exemption in eslint.config.js, tells Lab it may import Core's gesture, motion and touch-support source directly. No claim on main admits Lab to that source, and the INV13 text proposed in #8464 names @astryxdesign/core/internal/touch as the way Core and Lab components reach those modules. The engineering-owner decision on this pull request should settle whether Lab keeps that direct access or the deep patterns also apply to packages/lab.
There was a problem hiding this comment.
Changes requested. The rule now has spec authority, since spec:AST-017 FR21 and architecture:react-component-runtime INV13 on main (#8464) limit Core's ./internal/* subpaths by lint to Core and Lab, but three forms still reach an internal subpath from outside Core and Lab with lint green. Please close each one, with a test for the two rule changes:
- report an
import()whose specifier is a template literal with no expressions (no-core-internal-import.js:59-63); - report a relative path into
core/src/internal, as the rule already does forcore/src/gestures(no-core-internal-import.js:31); - add
mts,ctsto the config block'sfiles(eslint.config.js:265), since.mtsfiles are linted but not checked.
[Automated review]
| } | ||
| } | ||
| function checkLiteral(node) { | ||
| if (node != null && node.type === 'Literal') { |
There was a problem hiding this comment.
import(`@astryxdesign/core/internal/reorder`) lints clean from apps/sandbox/src: only a Literal specifier is checked here. Also accepting a TemplateLiteral whose expressions is empty (its one quasi's value.cooked), with an invalid test, closes it.
| // their own: by package path, or by a path into Core's source. | ||
| const DEEP = [ | ||
| /^@astryxdesign\/core\/(?:gestures|motion)(?:\/|$)/, | ||
| /(?:^|\/)core\/src\/(?:gestures|motion|touch-support)(?:\/|$)/, |
There was a problem hiding this comment.
A relative import of packages/core/src/internal/<name>, the file the README puts each subpath in, lints clean from apps, while the same reach into core/src/gestures is reported. Apps already import Core source by relative path (for example apps/sandbox/src/app/(sandbox)/pages/motion-lab/proposed/TabList/TabList.tsx:63), so covering core/src/internal here, reported with the internal message and an invalid test, closes a likely route around the fence.
| // Core's internal subpaths (`@astryxdesign/core/internal/*`) and its | ||
| // gesture, motion and touch-support source are for Core and Lab only. | ||
| { | ||
| files: ['**/*.{ts,tsx,js,jsx,mjs,cjs}'], |
There was a problem hiding this comment.
.mts files are linted (today apps/sandbox/vite.config.mts) but this glob does not match them, so an internal import in one passes. Adding mts,cts keeps the README's "every linted file" sentence true.
…internal From CodeBunny on #8518: a path into Core's source covered gestures, motion and touch-support but not internal, so '../../packages/core/src/internal/touch' from a story linted clean while re-exporting the same gesture core. The source-path pattern adds internal, with an invalid case (and a valid 'core/src/internals' that must not match); the message, header and README say so.
|
/approve-spec 27007e5 |
There was a problem hiding this comment.
Changes requested: the rule matches spec:AST-017 FR21 and architecture:react-component-runtime INV13 on main, and this head now reports relative paths into core/src/internal, but two routes still reach an internal subpath from outside Core and Lab with lint green.
- Report an
import()orrequire()whose specifier is a template literal with no expressions (no-core-internal-import.js:60-64), with a test for each form. - Add
mts,ctsto the config block'sfiles(eslint.config.js:265), since.mtsfiles such asapps/sandbox/vite.config.mtsare linted but not checked.
[Automated review]
…s and lints .mts and .cts From CodeBunny's review at 27007e5: an import() or require() named by a template literal with no expressions is a string too, so the rule reports it (a test for each form); the repo config block now lints .mts and .cts files, such as apps/sandbox/vite.config.mts.
|
/approve-spec 69a37ca |
There was a problem hiding this comment.
This is ready to merge: the rule matches spec:AST-017 FR21 and architecture:react-component-runtime INV13 on main, which limit Core's ./internal/* subpaths by lint to Core and Lab, and it reports every import form and file type its README lists while leaving Core and Lab alone. Two optional follow-ups:
- In the
TSImportTypevisitor (no-core-internal-import.js:104), readnode.sourcefirst, sinceargumentis deprecated in the repository's typescript-eslint 8.62 and prints aDeprecationWarning. - In the README's coverage sentence, name the
.mjsexclusion: every.mjsfile outsidepackages/cli's linted directories is unlinted, not only some scripts, so an internal import there passes.
[Automated review]
| // `typeof import('…')` in TypeScript types. typescript-eslint wraps | ||
| // the specifier in a TSLiteralType. | ||
| TSImportType(node) { | ||
| const argument = node.argument; |
There was a problem hiding this comment.
Non-blocking: in the repository's typescript-eslint 8.62.1, TSImportType carries the specifier in source, and argument is a deprecated alias that emits a DeprecationWarning when read (this rule's suite prints four). Reading source first keeps the typeof import() check working once the alias is removed:
TSImportType(node) {
if (node.source != null) {
checkLiteral(node.source);
return;
}
const argument = node.argument;
checkLiteral(argument?.type === 'TSLiteralType' ? argument.literal : argument);
},| `export … from`, `import()`, `require()` and `typeof import()`, a dynamic | ||
| import or require named by a string or a template literal with no expressions. The repo config | ||
| turns it on for every linted file outside `packages/core` and `packages/lab` | ||
| (the global ignores still leave some scripts unlinted), so only Core and Lab |
There was a problem hiding this comment.
Non-blocking: the global '**/*.mjs' ignore in eslint.config.js leaves every .mjs file outside packages/cli's listed directories unlinted (about 160 in apps/, packages/ and internal/, including modules such as apps/docsite/src/lib/*.mjs), not only scripts, so an internal import in one passes. Naming that exclusion here keeps the sentence accurate.
This pull request carried facebook#8561's earlier heads as a dependency. facebook#8561 is on main now, so the lint rule's files (eslint.config.js, the plugin's README, index.js, no-core-internal-import.js and its test) are main's, byte for byte; this pull request's own change (the internal entries, their tests, the export map, sync-exports and two changesets) is unchanged.
… into uikit/c09-search-field
User impact
Nobody outside Astryx starts depending on modules that can change without notice. Core is about to export a few undocumented modules under
@astryxdesign/core/internal/<name>, so Lab can build on machinery that has no public API of its own. The reorder engine behindcreateReorderablecomes first, for TransferList's staged selector and ListInput (#8143, #8163). These modules are reserved for Astryx's own packages and sit outside the compatibility promise (spec:AST-017FR21). This keeps everything else in the repo (apps, templates, other packages) from importing them.Dependency
This rule fences the
./internal/*export channel that #8464 adds tospec:AST-017FR21 andarchitecture:public-component-apiINV12, which the design owner has decided. It lands after #8464, or with an exact-head owner approval that cites it.Problem and solution fit
The rule also reports deep imports of Core's gesture core, motion and touch support (
@astryxdesign/core/gestures|motion(/*),…/core/src/{gestures,motion,touch-support}/…), which have no entry of their own (architecture:react-component-runtimeINV13), and every form:import,export … from,import(),require(),typeof import().A new rule,
@astryx/no-core-internal-import, reports any import, re-export or dynamic import of@astryxdesign/core/internalor a path under it. The repo config turns it on for every file outsidepackages/coreandpackages/lab.It's a custom rule, not a repo-wide
no-restricted-importsblock. Flat config overrides a same-named rule per file, so such a block would undo the layer blocks that already useno-restricted-imports.The plugin README records the convention each internal subpath follows:
"./internal/<name>": {source, types, default}exports entry, never a wildcard;src/internal/<name>/index.tsheader saying it's reserved, undocumented, and outside the promise.No internal subpath exists yet; the first one arrives with feat(lab): TransferList reorders through core's createReorderable #8143. Lint-only, so no changeset.
Evidence
no-core-internal-import.test.mjs: 9 cases. Public subpaths, look-alike paths and relative./internalpaths are allowed. Import, re-export,export *, dynamic import and the bareinternalpath are reported.apps/sandboxis reported; the same import inpackages/labisn't.