Skip to content
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.

Sign up for GitHub

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

Make sure forbidden-imports rule checks files directly inside layers #64

Merged
merged 10 commits into from
Jul 25, 2024
Merged

Make sure forbidden-imports rule checks files directly inside layers #64

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b4dde41
Make sure forbidden-imports rule checks files directly inside layers
daniilsapa Jul 21, 2024
935407c
Add a change log
daniilsapa Jul 21, 2024
d8c063d
Fix the problem of traversing the layer filesystem tree 2 times
daniilsapa Jul 22, 2024
92ed506
Fix constant declaration placement
daniilsapa Jul 23, 2024
737ed42
Fix the positive test case for forbidden-imports
daniilsapa Jul 23, 2024
892f5de
Cover changes in indexSourceFiles with tests
daniilsapa Jul 23, 2024
249e5b2
Improve parseIntoFsdRoot function by introducing the capability to sp…
daniilsapa Jul 24, 2024
6d512bc
Use the optional rootPath argument instead of directly accessing chil…
daniilsapa Jul 24, 2024
5731cc9
Fix the problem with failing test on Windows
daniilsapa Jul 24, 2024
74c02d1
Simplify parseIntoFsdRoot test utility
daniilsapa Jul 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/steiger-plugin-fsd/src/_lib/index-source-files.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ export function indexSourceFiles(root: Folder): Record<string, SourceFile> {
}

for (const [layerName, layer] of Object.entries(getLayers(root))) {
// Even though files that are directly inside a layer are not encouraged by the FSD and are forbidden in most cases
// (except for an index/root file for the app layer as an entry point to the application), users can still add them.
// So, we need to index all files directly inside a layer to find errors.
walk(layer, { layerName: layerName as LayerName, sliceName: null, segmentName: null })
daniilsapa marked this conversation as resolved.
Show resolved Hide resolved

if (!isSliced(layer)) {
for (const [segmentName, segment] of Object.entries(getSegments(layer))) {
walk(segment, { layerName: layerName as LayerName, sliceName: null, segmentName })
Expand Down
197 changes: 132 additions & 65 deletions packages/steiger-plugin-fsd/src/forbidden-imports/index.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,24 @@
import { expect, it, vi } from 'vitest'
import { Folder } from '@steiger/types'

import { joinFromRoot, parseIntoFsdRoot } from '../_lib/prepare-test.js'
import forbiddenImports from './index.js'

vi.mock('tsconfck', async (importOriginal) => {
return {
...(await importOriginal<typeof import('tsconfck')>()),
parse: vi.fn(() => Promise.resolve({ tsconfig: { compilerOptions: { paths: { '@/*': ['/*'] } } } })),
parse: vi.fn(() =>
Promise.resolve({
tsconfig: {
compilerOptions: {
baseUrl: '/src/',
paths: {
'@/*': ['./*'],
},
},
},
}),
),
}
})

Expand All @@ -16,103 +28,158 @@ vi.mock('node:fs', async (importOriginal) => {

return createFsMocks(
{
'/shared/ui/styles.ts': '',
'/shared/ui/Button.tsx': 'import styles from "./styles";',
'/shared/ui/TextField.tsx': 'import styles from "./styles";',
'/shared/ui/index.ts': '',
'/entities/user/ui/UserAvatar.tsx': 'import { Button } from "@/shared/ui"',
'/entities/user/index.ts': '',
'/entities/product/ui/ProductCard.tsx': 'import { UserAvatar } from "@/entities/user"',
'/entities/product/index.ts': '',
'/features/comments/ui/CommentCard.tsx': 'import { styles } from "@/pages/editor"',
'/features/comments/index.ts': '',
'/pages/editor/ui/styles.ts': '',
'/pages/editor/ui/EditorPage.tsx': 'import { Button } from "@/shared/ui"; import { Editor } from "./Editor"',
'/pages/editor/ui/Editor.tsx': 'import { TextField } from "@/shared/ui"',
'/pages/editor/index.ts': '',
'/src/shared/ui/styles.ts': '',
'/src/shared/ui/Button.tsx': 'import styles from "./styles";',
'/src/shared/ui/TextField.tsx': 'import styles from "./styles";',
'/src/shared/ui/index.ts': '',
'/src/entities/user/ui/UserAvatar.tsx': 'import { Button } from "@/shared/ui"',
'/src/entities/user/index.ts': '',
'/src/entities/product/ui/ProductCard.tsx': 'import { UserAvatar } from "@/entities/user"',
'/src/entities/product/index.ts': '',
'/src/entities/cart/ui/SmallCart.tsx': 'import { App } from "@/app"',
'/src/entities/cart/lib/count-cart-items.ts': 'import root from "@/app/root.ts"',
'/src/entities/cart/lib/index.ts': '',
'/src/entities/cart/index.ts': '',
'/src/features/comments/ui/CommentCard.tsx': 'import { styles } from "@/pages/editor"',
'/src/features/comments/index.ts': '',
'/src/pages/editor/ui/styles.ts': '',
'/src/pages/editor/ui/EditorPage.tsx': 'import { Button } from "@/shared/ui"; import { Editor } from "./Editor"',
'/src/pages/editor/ui/Editor.tsx': 'import { TextField } from "@/shared/ui"',
'/src/pages/editor/index.ts': '',
'/src/app': '',
'/src/app/ui/index.ts': '',
'/src/app/index.ts': '',
'/src/app/root.ts': '',
},
originalFs,
)
})

it('reports no errors on a project with only correct imports', async () => {
const root = parseIntoFsdRoot(`
📂 shared
📂 ui
📄 styles.ts
📄 Button.tsx
📄 TextField.tsx
📄 index.ts
📂 pages
📂 editor
📂 src
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: the idea is to give the check function of rules the folder of the FSD root, so by putting the layers in src, we're not actually testing how it's going to be

suggestion: we can add a second optional argument to parseIntoFsdRoot — the root path of the folders, then we can avoid having src here in the structure

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, now I see that in other tests, you access .children[0] to overcome this issue. That's also fine, although you forgot to do it in the positive case. I still think that it would be cleaner with an optional root path argument though

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I had to put all the files into /src because of the problem I wrote about in the PR description. Indeed I forgot to add .children[0] for the positive case, thanks for catching that.

I would be happy to pass that optional argument to parseIntoFsdRoot but I see that it accepts only 1 argument. Maybe you implemented the second parameter in some branch that has not yet been merged to the master?

Screenshot 2024-07-23 at 22 36 08

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to suggest for you to add that second parameter :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also took the liberty to unresolve this conversation. Usually, I prefer to mark threads as resolved when the comment was either fully addressed or it was decided not to address it, but this comment is still relevant partially. Let me know if you'd like to add that second parameter, or I can do it

Copy link
Collaborator Author

@daniilsapa daniilsapa Jul 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, got it. No problem, I'll add that tomorrow. I think it will be useful in further test cases and also I don't like .children[0] too much.

📂 shared
📂 ui
📄 EditorPage.tsx
📄 Editor.tsx
📄 index.ts
📄 styles.ts
📄 Button.tsx
📄 TextField.tsx
📄 index.ts
📂 pages
📂 editor
📂 ui
📄 EditorPage.tsx
📄 Editor.tsx
📄 index.ts
`)

expect((await forbiddenImports.check(root)).diagnostics).toEqual([])
})

it('reports errors on a project with cross-imports in entities', async () => {
const root = parseIntoFsdRoot(`
📂 shared
📂 ui
📄 styles.ts
📄 Button.tsx
📄 TextField.tsx
📄 index.ts
📂 entities
📂 user
📂 src
📂 shared
📂 ui
📄 UserAvatar.tsx
📄 index.ts
📂 product
📂 ui
📄 ProductCard.tsx
📄 index.ts
📂 pages
📂 editor
📂 ui
📄 EditorPage.tsx
📄 Editor.tsx
📄 index.ts
📄 styles.ts
📄 Button.tsx
📄 TextField.tsx
📄 index.ts
📂 entities
📂 user
📂 ui
📄 UserAvatar.tsx
📄 index.ts
📂 product
📂 ui
📄 ProductCard.tsx
📄 index.ts
📂 pages
📂 editor
📂 ui
📄 EditorPage.tsx
📄 Editor.tsx
📄 index.ts
`)

expect((await forbiddenImports.check(root)).diagnostics).toEqual([
expect((await forbiddenImports.check(root.children[0] as Folder)).diagnostics).toEqual([
{
message: `Forbidden cross-import from slice "user".`,
location: { path: joinFromRoot('entities', 'product', 'ui', 'ProductCard.tsx') },
location: { path: joinFromRoot('src', 'entities', 'product', 'ui', 'ProductCard.tsx') },
},
])
})

it('reports errors on a project where a feature imports from a page', async () => {
const root = parseIntoFsdRoot(`
📂 shared
📂 ui
📄 styles.ts
📄 Button.tsx
📄 TextField.tsx
📄 index.ts
📂 features
📂 comments
📂 src
📂 shared
📂 ui
📄 CommentCard.tsx
📄 index.ts
📂 pages
📂 editor
📄 styles.ts
📄 Button.tsx
📄 TextField.tsx
📄 index.ts
📂 features
📂 comments
📂 ui
📄 CommentCard.tsx
📄 index.ts
📂 pages
📂 editor
📂 ui
📄 styles.ts
📄 EditorPage.tsx
📄 Editor.tsx
📄 index.ts
`)

expect((await forbiddenImports.check(root.children[0] as Folder)).diagnostics.sort()).toEqual([
{
message: `Forbidden import from higher layer "pages".`,
location: { path: joinFromRoot('src', 'features', 'comments', 'ui', 'CommentCard.tsx') },
},
])
})

it('reports errors in a project where a lower level imports from files that are direct children of a higher level', async () => {
const root = parseIntoFsdRoot(`
📂 src
📂 shared
📂 ui
📄 styles.ts
📄 EditorPage.tsx
📄 Editor.tsx
📄 Button.tsx
📄 TextField.tsx
📄 index.ts
📂 entities
📂 cart
📄 index.ts
📂 lib
📄 count-cart-items.ts
📄 index.ts
📂 ui
📄 SmallCart.tsx
📂 pages
📂 editor
📂 ui
📄 styles.ts
📄 EditorPage.tsx
📄 Editor.tsx
📄 index.ts
📂 app
📂 ui
📄 index.ts
📄 index.ts
📄 root.ts
`)

expect((await forbiddenImports.check(root)).diagnostics).toEqual([
const diagnostics = (await forbiddenImports.check(root.children[0] as Folder)).diagnostics
expect(diagnostics).toEqual([
{
message: `Forbidden import from higher layer "pages".`,
location: { path: joinFromRoot('features', 'comments', 'ui', 'CommentCard.tsx') },
message: `Forbidden import from higher layer "app".`,
location: { path: joinFromRoot('src', 'entities', 'cart', 'lib', 'count-cart-items.ts') },
},
{
message: `Forbidden import from higher layer "app".`,
location: { path: joinFromRoot('src', 'entities', 'cart', 'ui', 'SmallCart.tsx') },
},
])
})
3 changes: 2 additions & 1 deletion packages/steiger-plugin-fsd/src/forbidden-imports/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import * as fs from 'node:fs'
import { layerSequence, resolveImport } from '@feature-sliced/filesystem'
import precinct from 'precinct'
const { paperwork } = precinct
daniilsapa marked this conversation as resolved.
Show resolved Hide resolved
import { parse as parseNearestTsConfig } from 'tsconfck'
import type { Diagnostic, Rule } from '@steiger/types'

import { indexSourceFiles } from '../_lib/index-source-files.js'

const { paperwork } = precinct

const forbiddenImports = {
name: 'forbidden-imports',
async check(root) {
Expand Down