feat: add the no-wildcard-exports rule - #252
Conversation
🦋 Changeset detectedLatest commit: 3074622 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
I like how we can add many new rules now using tree-sitter, but I don't think adding random flags and queries into extractors is 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)
- make
extractDependenciesreturn something like
function extractDependencies(): Statement[]
// I'm not sure if the types are correct, need to veryfy how wildcards and namespaces work together
// this also doesn't take into account import renaming and import attributes
type Statement = Import | Export;
interface Import {
specifier: string
path: string;
}
interface Export {
namespace?: string
specifier: string
path: string
}- save all import/export information back to the FS cache so we don't have to re-parse the file
- add framework-specific logic for exports: for example, Vue files with the
setupattribute by default have one implicit default export (I believe this is also the case for svelte) - (optionally) change the querying options for the
extractDependenciesso we don't have to add too much filtering logic into the rules
There was a problem hiding this comment.
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.
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?
| /** | ||
| * 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 |
There was a problem hiding this comment.
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";There was a problem hiding this comment.
export * as foo from "./foo";
export type * as foo from "./foo";
After hearing your feedback, I think it would be better to handle this part as well. Thank you for the review.
Background
Added a
no-wildcard-exportsrule that detects wildcard re-exports in public APIs.It reports the following patterns:
Explicit re-exports are still allowed:
To support this,
_language-toolswas refactored to analyze imports and re-exports together and reuse the same FS cache. The behavior of existing import-based rules remains unchanged.