feat(steiger-plugin): add no-wildcard-exports rule and fix tsconfig resolution#215
feat(steiger-plugin): add no-wildcard-exports rule and fix tsconfig resolution#215Gaic4o wants to merge 3 commits intofeature-sliced:masterfrom
Conversation
|
illright
left a comment
illright
left a comment
There was a problem hiding this comment.
Thanks for the great work! You covered nearly all aspects of creating a rule all by yourself :) The last things to do are to packages/steiger-plugin-fsd/src/index.ts and add it to disabledRules (we don't want to enable it by default for now not to break people's CI, we will enable it by default in 0.6, with the next round of breaking changes) and also add it to the table of rules in the README (note that the README is copy-pasted between the project root and packages/steiger, so both files need to be updated).
| if (!/\.(js|jsx|ts|tsx)$/.test(file.path)) continue | ||
|
|
||
| // Allow wildcard exports in unsliced layers (shared, app) | ||
| if (sourceFile.layerName === 'shared' || sourceFile.layerName === 'app') continue |
There was a problem hiding this comment.
suggestion: let's use isSliced from @feature-sliced/filesystem to keep this logic centralized
| const fileName = basename(file.path) | ||
| const isPublicApiFile = /^index\.(js|jsx|ts|tsx)$/.test(fileName) |
There was a problem hiding this comment.
This is also important because we allow index.client.ts and index.server.ts as valid Public APIs, and this function takes that into account
| if (!isPublicApiFile) continue | ||
|
|
||
| // Parse file content using oxc-parser | ||
| const content = (file as FileWithContent).content |
There was a problem hiding this comment.
question: why is the type-cast here necessary?
There was a problem hiding this comment.
I just realized — we don't include content as a field on the VFS node, we might need to explicitly read the file contents with readFile
| const content = (file as FileWithContent).content | ||
| if (!content) continue // Skip if file has no contents | ||
|
|
||
| const parseResult = parseSync(file.path, content) |
There was a problem hiding this comment.
suggestion: rules can be asynchronous, so we can use the async version of the parser
| fixes: [ | ||
| { | ||
| type: 'modify-file', | ||
| path: file.path, | ||
| content: | ||
| '// Replace with named exports\n// Example: export { ComponentA, ComponentB } from "./components"', | ||
| }, | ||
| ], |
There was a problem hiding this comment.
issue: I don't think that's a great autofix — ideally, autofixes should be safe code changes that you can apply and not worry about your code breaking. This will probably break code
suggestion: let's not offer an autofix for this rule until we're able to determine what names are being wildcard-exported (which is a worthwhile improvement as a second iteration of this rule)
|
|
||
| Forbid wildcard exports (`export *`) in public APIs of business logic layers. Named exports and namespace exports (`export * as namespace`) are allowed. | ||
|
|
||
| **Exception:** Wildcard exports are allowed in unsliced layers (`shared` and `app`), as they serve as foundational layers with different architectural purposes. |
There was a problem hiding this comment.
issue: I would actually argue against allowing wildcard exports even in Shared and App. To me, wildcard exports conceal the public API of a group of modules, which really hurts the ability to discover a codebase shallowly, i. e. without digging into every file.
question: why do you think they should be allowed there? And do you agree with the content of the Public API reference page in the FSD docs?
| - Unintentionally exposing internal implementation details | ||
| - Difficulty in tracking dependencies between modules | ||
| - Potential naming conflicts when multiple modules use wildcard exports |
There was a problem hiding this comment.
suggestion: I would also add the point about hurting shallow discovery (if you agree with it, that is :D)
| - Difficulty in tracking dependencies between modules | ||
| - Potential naming conflicts when multiple modules use wildcard exports | ||
|
|
||
| Using named exports or namespace exports makes the public API more explicit and easier to maintain in business logic layers. |
There was a problem hiding this comment.
suggestion: let's also leave a link to the docs about Public API
| "@feature-sliced/filesystem": "^3.0.1", | ||
| "fastest-levenshtein": "^1.0.16", | ||
| "lodash-es": "^4.17.21", | ||
| "oxc-parser": "^0.47.1", |
There was a problem hiding this comment.
praise: kudos for using the oxc-parser :) I was hoping that we would switch to it entirely at some point and drop precinct because it isn't flexible enough and doesn't extend well
There was a problem hiding this comment.
By the way, it supposedly provides a list of exports in a separate list that's easily accessible, can we use that? See "Returns ESM information" in https://www.npmjs.com/package/oxc-parser
| function addContentToFiles(folder: Folder): void { | ||
| for (const child of folder.children) { | ||
| if (child.type === 'file') { | ||
| const fileWithContent = child as FileWithContent | ||
| if (child.path === joinFromRoot('shared', 'ui', 'index.ts')) { | ||
| fileWithContent.content = "export * as positions from './positions'" | ||
| } else if (child.path === joinFromRoot('entities', 'user', 'index.ts')) { | ||
| fileWithContent.content = "export * as userModel from './model'" | ||
| } else { | ||
| fileWithContent.content = '' | ||
| } | ||
| } else if (child.type === 'folder') { | ||
| addContentToFiles(child) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
issue: I think this conceals the fact that this test will actually not pass during real usage, since the linter engine doesn't include the content field
suggestion: let's use FS mocking for tests, like it's done in the test file for forbidden-imports
2 participants
Background
no-wildcard-exportsrule in@feature-sliced/steiger-pluginexport *in public APIs of business layersexport * as namespaceand wildcard exports inshared/appindex.(js|jsx|ts|tsx);non-index and test files are ignoredREADME.mdfor the rule@tsconfig/node18todependenciesin@steiger/tsconfigfor proper tsconfig extends resolution