Repository navigation
chore(lint): flag package-internal imports through a barrel - #9080
Conversation
A new @astryx/no-internal-barrel-import rule reports a relative import in
a package's shipped source that resolves to a folder's index instead of
the defining module ('../List' rather than '../List/List'). A lazily
imported module that goes through a barrel pulls every sibling export into
its graph, which broke the single-file bundle (#9055). Type-only imports
are allowed; public consumers keep importing @astryxdesign/core.
The 796 barrel imports already on main are listed by file in a baseline
and stay allowed until a later codemod removes them, so main stays green
while a pull request that adds a new one fails strict lint. AGENTS.md
gets a one-line rule; the plugin README documents it.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewNo new or modified components detected. Bundle Size SummaryNo component packages changed. Accessibility AuditStatus: The accessibility audit did not run on this pull request. It runs after merges to Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
|
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.
Thanks, the rule does what it describes and keeps main lint-green, but two scope problems need fixing before it lands.
- It also covers package entries, so the
export * from './MyComponent'step inCONTRIBUTING.mdnow fails strict lint for every new component; leavingindex.*files out of the rule (and dropping their baseline entries) keeps the lazy-graph protection. - Baseline keys are resolved against the working directory, so each package's own
eslint srcscript reports every baselined import (782 in core); resolve them against the repository root instead.
Explicit '../X/index' specifiers also slip past the rule and are worth covering in the same pass.
[Automated review]
| // lint, while main stays green). Type-only imports are erased and allowed. | ||
| { | ||
| files: ['packages/*/src/**/*.{ts,tsx,mts,cts,js,jsx,mjs}'], | ||
| ignores: INTERNAL_BARREL_TEST_FILES, |
There was a problem hiding this comment.
This block also covers package entries and component barrels. Adding export * from './Code'; to packages/core/src/index.ts, the step CONTRIBUTING.md prescribes for a new component, fails strict lint ('./Code' resolves to a folder's index). Following the message there (export * from './Dialog/Dialog') would publish exports the barrel leaves out. Adding '**/index.*' to these ignores keeps the lazy-graph protection, because an ordinary module's barrel import is still reported, and lets the 13 index-file keys leave the baseline.
| const filename = context.filename; | ||
| const baseline = context.options[0]?.baseline ?? {}; | ||
| const relative = path | ||
| .relative(context.cwd, filename) |
There was a problem hiding this comment.
context.cwd is the directory ESLint was started from. Core, lab, charts and richtext run eslint src from their package directory, where these keys become src/... and miss the baseline: cd packages/core && ASTRYX_STRICT_LINT=1 eslint src/AlertDialog/AlertDialog.tsx reports 5 errors, and the whole package reports 782. Passing the repository root from eslint.config.js (for example {baseline, root: import.meta.dirname}) and computing the key relative to it would fix this.
| if (isFile(target)) { | ||
| return false; | ||
| } | ||
| if (MODULE_EXTENSIONS.some(extension => isFile(target + extension))) { |
There was a problem hiding this comment.
An explicit index specifier passes here: '../Button/index' resolves to index.ts through this check, and '../Button/index.ts' and '../Button/index.js' are accepted too. A lazy import('../List/index') would reach the barrel unreported. Treating a last segment of index or index.<ext> as a barrel closes the gap.
…ch '/index' specifiers - A barrel itself (a folder's or a package's index) is the public surface and may re-export from other barrels, so the rule skips index files and the documented component-export step keeps passing. - Baseline keys are relative to the repository root, passed from the config, so 'eslint src' run from a package directory finds them. - '../List/index' and '../List/index.ts' resolve to the barrel and are reported too.
…imports, 187 files)
There was a problem hiding this comment.
The rule and the fixes from the last round work as described, but the Lint check fails on this head and needs two fixes before it can land.
eslint.config.js:39usesURL, which the config file does not declare as a global, sopnpm lintreports'URL' is not defined; reading the baseline withpath.join(import.meta.dirname, …), as the.gitignoreimports do, avoids it.- #9057 added
import {useTranslator} from '../../../i18n'inuseTableColumnResize.tsxonmain, so the baseline needs regenerating after mergingmain(or that import can point at../../../i18n/useTranslator).
'…/index.js' specifiers still pass the explicit-index check, and the new config block still sits inside richtext's backlog comment; both are worth fixing in the same pass.
[Automated review]
| // by repo-relative file. The list only shrinks. | ||
| const internalBarrelBaseline = JSON.parse( | ||
| readFileSync( | ||
| new URL( |
There was a problem hiding this comment.
URL is not declared for this file (only /* global process */ at line 13), so pnpm lint reports 'URL' is not defined (no-undef) here in strict and local mode; the Lint check for this head fails on it. Reading the file the way the .gitignore imports do avoids the global: readFileSync(path.join(import.meta.dirname, 'internal/eslint-plugin-astryx/no-internal-barrel-import.baseline.json'), 'utf8').
| const base = path.basename(target); | ||
| if ( | ||
| (base === 'index' && MODULE_EXTENSIONS.some(e => isFile(target + e))) || | ||
| (INDEX_FILES.includes(base) && isFile(target)) |
There was a problem hiding this comment.
This only matches an index basename that exists as a file, so '../i18n/index.js' (resolved to index.ts by TypeScript) still passes, and in native, which writes .js specifiers, that is the form a barrel import takes ('../components/index.js' from a non-index module reports nothing). Reporting a basename of index with any module extension when the directory holds any index.* would close it.
…' specifiers
- eslint.config.js reads the baseline through path.join(import.meta.dirname)
like its .gitignore imports, so no-undef stays quiet.
- A specifier naming index with any module extension ('../i18n/index.js',
which TypeScript resolves to index.ts) is reported when the folder holds
an index file.
There was a problem hiding this comment.
The fixes from the last round work: the config lints clean and '…/index.js' specifiers are now reported, but the Lint check still fails on the merge with main. #9023 landed import {useTranslator} from '../i18n' at packages/core/src/Slider/Slider.tsx:62 after this head was pushed, so the baseline needs regenerating after merging main again, ideally right before landing so it does not drift a third time. The new config block also still sits inside richtext's backlog comment (eslint.config.js:878-892); moving it above that comment in the same pass keeps the comment with its block.
[Automated review]
| "../utils" | ||
| ], | ||
| "packages/core/src/Skeleton/Skeleton.tsx": ["../utils"], | ||
| "packages/core/src/Slider/Slider.tsx": [ |
There was a problem hiding this comment.
#9023 added import {useTranslator} from '../i18n'; at packages/core/src/Slider/Slider.tsx:62 on main, and this entry does not list '../i18n', so the Lint check on the merge with main fails with that one error. Regenerating after merging main adds it; nothing else differs.
| // | ||
| // Remove an entry here as its backlog closes; the file is clean when the | ||
| // block is empty. | ||
| // Inside a package's own shipped source, import the module that defines a |
There was a problem hiding this comment.
This block lands between richtext's backlog comment (lines 878-892) and the richtext block at line 914, so that comment now reads as if it describes the barrel rule. Moving this block and its comment above line 878 keeps richtext's block last and its comment attached.
…lock last - no-internal-barrel-import.baseline.mjs rewrites (or --check verifies) the baseline from the source with the repo config, so it can follow main and shrink after a codemod. - The rule's config block moves above richtext's backlog comment, which stays attached to the last block.
The rule module exports the files and ignores the repo config uses, so the baseline script lints exactly the same files (skipping packages whose source the config ignores). The baseline picks up the one barrel import main gained since (Slider's '../i18n').
There was a problem hiding this comment.
The barrel-import rule, its shared file scope and the baseline script work as described, and the points from the last round are resolved: the baseline now lists the Slider.tsx import and the config block sits above richtext's backlog comment. The Lint check passes on the merge with main, and the rule's tests pass. If main gains another barrel import before this lands, merge it and rerun the baseline script, as the description plans.
[Automated review]
Maintainer impact
A module inside an Astryx package that imports through a folder's index file (a barrel, e.g.
import {List} from '../List') pulls every sibling export of that folder into its module graph. In lazily imported graphs this broke the single-file bundle (#9055 fixes the crash by importing defining modules). Nothing stops a new barrel import from bringing it back: there are about 600 such imports on main, and new code copies them.Owner decision: Cindy Zhang approved this rule on 2026-10-30 ("yes import rule"). Inside Astryx's own packages, import the defining module, never a barrel; public consumers keep importing from
@astryxdesign/core. No knowledge record owns internal import paths, so this PR adds none.Intended invariant
packages/*/srcimports defining modules ('../List/List'), not folder indexes ('../List').mainstays lint-green: only new barrel imports are reported.Change
@astryx/no-internal-barrel-import(internal/eslint-plugin-astryx/). It reports a relativeimport,export … fromor literalimport()that resolves to a directory with anindex.*file and no same-named module. Type-only imports and exports are allowed, since the compiler erases them. Package specifiers such as@astryxdesign/coreare never read.packages/*/src, excluding tests, stories and test helpers (they never ship) and index files themselves. A barrel is the public surface (architecture:public-component-api: component barrels own reachable exports) and may re-export from other barrels, so addingexport * from './Code'to a package entry, as CONTRIBUTING.md prescribes, still passes. It uses the repo's two-tier severity: an error under strict lint (CI), a warning locally. Naming the index itself ('../List/index','../List/index.ts', or'../List/index.js', which TypeScript resolves toindex.ts) is reported too.no-internal-barrel-import.baseline.jsonand stay allowed. A new barrel import, a new file or a new specifier in an existing file is reported. Keys are relative to the repository root, passed from the config, soeslint srcrun inside a package finds the same entries. The list only shrinks; a later codemod clears it.node internal/eslint-plugin-astryx/no-internal-barrel-import.baseline.mjsrewrites it from the source with the same file scope as the config (--checkverifies it).no-nullish-jsx-guarduses) would not stop a pull request, and a diff-scoped CI step would change workflows.AGENTS.md's Quick Reference, and a rule section in the plugin README.import {List} from '../List';in a new or changed module'../List' resolves to a folder's index (a barrel)…import {List} from '../List/List';import type {ListProps} from '../List';import {List} from '@astryxdesign/core';export * from './Code'in a package'sindex.tsEvidence
import {List} from '../List'lints clean on main. With this rule, strict lint reports it with the message above.ASTRYX_STRICT_LINT=1 eslint packages/{core,lab,charts,richtext,vega}/srcreports 0 errors (1,879 files),cd packages/core && eslint srcreports no barrel errors, fullASTRYX_STRICT_LINT=1 pnpm lintreports 0 errors, andpnpm check:repopasses.Scope
Testing
pnpm vitest run internal/eslint-plugin-astryx/no-internal-barrel-import.test.mjs: 23 cases. A temp package on disk covers'../List'(flagged);'../List/List','../utils'(a file), a folder with no index,@astryxdesign/coreand type-only imports (allowed);export … from,export *, literalimport()and'..'(flagged); explicit'../List/index'and'../List/index.ts'(flagged); and the baseline allowing only the listed file and specifier, keyed fromroot.pnpm vitest run internal/eslint-plugin-astryx: 861 tests pass.Before merging, the baseline must match the merge base. If main gains or drops barrel imports in the meantime, I merge main in again and rerun the baseline script.