-
Notifications
You must be signed in to change notification settings - Fork 23
feat: add the no-wildcard-exports rule #252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@feature-sliced/steiger-plugin': minor | ||
| --- | ||
|
|
||
| add the `no-wildcard-exports` rule, which forbids `export * from` in public APIs but allows `export * as ns from`. It is disabled by default. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,6 +44,8 @@ interface Extractor { | |
| language: Language | ||
| injections: Array<{ query: Query; lang: string }> | ||
| queries: Array<{ query: Query; type: 'static' | 'dynamic' }> | ||
| /** Matches `export * from` statements, capturing the whole statement as `@statement` and the module specifier as `@path`. */ | ||
| wildcardExportQuery?: Query | ||
| } | ||
|
|
||
| const extractors: Array<Extractor> = [ | ||
|
|
@@ -90,6 +92,9 @@ const extractors: Array<Extractor> = [ | |
| type: 'dynamic', | ||
| }, | ||
| ], | ||
| // Deliberately does not match `export * as ns from`: a namespace re-export adds one name to the | ||
| // module's exports rather than an unknown number of them. | ||
| wildcardExportQuery: new Query(tsx, '(export_statement "*" source: (string (string_fragment) @path)) @statement'), | ||
| injections: [], | ||
| }, | ||
| { | ||
|
|
@@ -170,6 +175,46 @@ function processExtractor(extractor: Extractor, tree: Tree): Dependency[] { | |
| return result | ||
| } | ||
|
|
||
| function processWildcardExports(extractor: Extractor, tree: Tree): WildcardExport[] { | ||
| if (extractor.wildcardExportQuery === undefined) return [] | ||
|
|
||
| const result: WildcardExport[] = [] | ||
|
|
||
| for (const match of extractor.wildcardExportQuery.matches(tree.rootNode)) { | ||
| const pathCapture = match.captures.find((capture) => capture.name === 'path') | ||
| const statementCapture = match.captures.find((capture) => capture.name === 'statement') | ||
| if (pathCapture === undefined || statementCapture === undefined) continue | ||
|
|
||
| result.push({ | ||
| path: pathCapture.node.text, | ||
| start: { | ||
| line: statementCapture.node.startPosition.row + 1, | ||
| column: statementCapture.node.startPosition.column + 1, | ||
| }, | ||
| end: { | ||
| line: statementCapture.node.endPosition.row + 1, | ||
| column: statementCapture.node.endPosition.column + 1, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| return result | ||
| } | ||
|
|
||
| interface WildcardExport { | ||
| /** The module specifier that the names are re-exported from. */ | ||
| path: string | ||
| // all indexes are 1-based, and they span the whole `export * from` statement | ||
| start: { | ||
| line: number | ||
| column: number | ||
| } | ||
| end: { | ||
| line: number | ||
| column: number | ||
| } | ||
| } | ||
|
|
||
| interface Dependency { | ||
| path: string | ||
| builtIn: boolean | ||
|
|
@@ -185,22 +230,24 @@ interface Dependency { | |
| } | ||
| } | ||
|
|
||
| const cache = createFSCache<Dependency[]>() | ||
|
|
||
| function extractAllDependencies(path: string): Dependency[] { | ||
| /** | ||
| * Parse a source file and run `process` on its syntax tree, as well as on the syntax trees of the | ||
| * languages injected into it (for example, the `<script>` block of a Vue component). | ||
| */ | ||
| function processSourceFile<T>(path: string, process: (extractor: Extractor, tree: Tree) => Array<T>): Array<T> { | ||
| const extension = extname(path) | ||
| const extractor = extractors.find((extractor) => extractor.extensions.includes(extension)) | ||
| if (!extractor) throw new Error(`No extractor found for "${extension}"`) | ||
|
|
||
| const dependencies: Dependency[] = [] | ||
| const results: Array<T> = [] | ||
|
|
||
| const sourceCode = readFileSync(path, 'utf8') | ||
| const parser = new Parser() | ||
| parser.setLanguage(extractor.language) | ||
| const tree = parser.parse(sourceCode) | ||
| if (tree === null) return [] | ||
|
|
||
| dependencies.push(...processExtractor(extractor, tree)) | ||
| results.push(...process(extractor, tree)) | ||
|
|
||
| for (const { query, lang } of extractor.injections) { | ||
| const injectedExtractor = extractors.find((extractor) => extractor.type === lang) | ||
|
|
@@ -225,13 +272,19 @@ function extractAllDependencies(path: string): Dependency[] { | |
| parser.setLanguage(injectedExtractor.language) | ||
| const injectedTree = parser.parse(sourceCode, null, { includedRanges }) | ||
| if (injectedTree === null) continue | ||
| dependencies.push(...processExtractor(injectedExtractor, injectedTree)) | ||
| results.push(...process(injectedExtractor, injectedTree)) | ||
| injectedTree.delete() | ||
| } | ||
|
|
||
| tree.delete() | ||
|
|
||
| return dependencies | ||
| return results | ||
| } | ||
|
|
||
| const cache = createFSCache<Dependency[]>() | ||
|
|
||
| function extractAllDependencies(path: string): Dependency[] { | ||
| return processSourceFile(path, processExtractor) | ||
| } | ||
|
|
||
| export async function extractDependencies( | ||
|
|
@@ -258,3 +311,21 @@ export async function extractDependencies( | |
| return true | ||
| }) | ||
| } | ||
|
|
||
| const wildcardExportCache = createFSCache<WildcardExport[]>() | ||
|
|
||
| /** | ||
| * Find the wildcard re-exports (`export * from`) in a file. | ||
| * | ||
| * Namespace re-exports (`export * as ns from`) are not reported. They add one name to the module's | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this could be a bit destructive; for example this file will be considered a valid public API file, while not providing any of contract/abstraction // index.ts
export * as ui from "./ui";
export * as model from "./model";
export * as api from "./api";
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
After hearing your feedback, I think it would be better to handle this part as well. Thank you for the review. |
||
| * exports, so they don't hide what the module exports. | ||
| */ | ||
| export async function extractWildcardExports(path: string): Promise<WildcardExport[]> { | ||
| let wildcardExports = wildcardExportCache.get(path) | ||
| if (!wildcardExports) { | ||
| wildcardExports = processSourceFile(path, processWildcardExports) | ||
| wildcardExportCache.set(path, wildcardExports) | ||
| } | ||
|
|
||
| return wildcardExports | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| # `no-wildcard-exports` | ||
|
|
||
| Forbid wildcard re-exports (`export * from`) in public APIs. | ||
|
|
||
| According to the _public API rule on slices_: | ||
|
|
||
| > Every slice (and segment on layers that don't have slices) must contain a public API definition. | ||
| > | ||
| > Modules outside of this slice/segment can only reference the public API, not the internal file structure of the slice/segment. | ||
| > https://feature-sliced.design/docs/reference/slices-segments#public-api-rule-on-slices | ||
|
|
||
| The FSD documentation lists wildcard re-exports as bad practice: | ||
|
|
||
| > This hurts the discoverability of a slice because you can't easily tell what the interface of this slice is. | ||
| > https://feature-sliced.design/docs/reference/public-api#what-makes-a-good-public-api | ||
|
|
||
| This rule checks every public API file on every layer, including index variants like `index.client.ts` and `index.server.ts`. It skips every other file, because a wildcard export inside a module stays private to the slice or segment that contains it. | ||
|
|
||
| Namespace re-exports (`export * as ns from`) are allowed. They add one name to the public API, so a reader can still tell what the module exports. | ||
|
|
||
| Examples of public APIs that pass this rule: | ||
|
|
||
| ```ts | ||
| // entities/user/index.ts | ||
| export { UserCard } from './ui/UserCard' | ||
| export { type User, useUser } from './model/user' | ||
| ``` | ||
|
|
||
| ```ts | ||
| // shared/ui/index.ts | ||
| export { Form, Field } from './form' | ||
| export * as positions from './tooltip-positions' | ||
| ``` | ||
|
|
||
| Examples of public APIs that fail this rule: | ||
|
|
||
| ```ts | ||
| // entities/user/index.ts | ||
| export * from './ui/UserCard' // ❌ | ||
| export * from './model/user' // ❌ | ||
| ``` | ||
|
|
||
| ```ts | ||
| // shared/ui/index.ts | ||
| export { Form, Field } from './form' | ||
| export * from './tooltip-positions' // ❌ | ||
| ``` | ||
|
|
||
| ## Rationale | ||
|
|
||
| A wildcard re-export hides the public API of a group of modules, so you can't tell what a slice exports without opening every file inside it. | ||
|
|
||
| It also lets the public API change by accident. Adding an export to an internal module adds it to the public API too, and removing that export later breaks whoever started using it. Listing the names means the public API only changes when someone edits the index file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how we can add many new rules now using tree-sitter, but I don't think adding random flags and queries into
extractorsis the way to go. As this API was created specifically for getting imports only, and it doesn't really scale up to exports (especially with wildcards and namespaces). I propose we refactor this part a bit first to make sure that it handles both imports and exports simultaneously, and then add export-based rules on top of it. (this is also the case for #251)extractDependenciesreturn something likesetupattribute by default have one implicit default export (I believe this is also the case for svelte)extractDependenciesso we don't have to add too much filtering logic into the rulesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve updated the implementation based on your feedback, and namespace re-exports are now reported by this rule as well.
One thing I noticed while working on this is that issue #5 currently lists the following as an example that should pass:
export * as positions from "./tooltip-positions";This seems to conflict with the direction of the current review. For now, I’ve followed the current review and included namespace re-exports in the rule, but could you confirm whether this is the intended behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also looked into the framework specific implicit exports you mentioned. To implement this properly, I think I would need to spend some additional time looking into and verifying the behavior of Vue, Svelte.
Would you prefer to include this in the scope of this PR as well, or would it be better to keep this PR focused on the shared analysis structure and handle framework-specific implicit exports separately in a follow up?