Make sure forbidden-imports rule checks files directly inside layers #64
Conversation
Had to move all layers into the /src folder for the mock filesystem used in the unit tests for the rule because there was an edge case where ts.resolveModuleName could not correctly resolve aliases for folders that are directly at the root of the filesystem (it did not resolve /app/index.ts from @/app and same problem for all layers). After moving everything in /src, they resolve just fine. I thought about creating an issue in TS repository, but it seems like an extremely rare edge case that nobody would care about. No-one puts their real projects in the root of the file system.
|
@illright |
| 📄 index.ts | ||
| 📂 pages | ||
| 📂 editor | ||
| 📂 src |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I meant to suggest for you to add that second parameter :)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
…ecify a path to the root
…dren when the root is parsed
| function findSubfolder(folder: Folder, path: string): Folder { | ||
| function checkFolder(folder: Folder): Folder { | ||
| if (folder.path === path) { | ||
| return folder | ||
| } | ||
|
|
||
| if (path.startsWith(folder.path)) { | ||
| for (const child of folder.children) { | ||
| if (child.type === 'folder') { | ||
| const result = checkFolder(child) | ||
| if (result) { | ||
| return result | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| throw new Error(`Path "${path}" not found in the provided file system mock!`) | ||
| } | ||
|
|
||
| return checkFolder(folder) | ||
| } |
There was a problem hiding this comment.
suggestion: I meant a more simple solution — you pass the root path as the second argument, and whatever file tree you write with emojis is "mounted" to that root. For example:
parseIntoFsdRoot(`
📂 test
📄 file
`, "/home/illright"); // the "file" becomes `/home/illright/test/file`There was a problem hiding this comment.
Yeah, I agree, the simpler the better, the KISS principle should not be forgotten 😂
|
No problem, thanks for your reviews and feedback! |
Resolves #49
Had to move all layers into the /src folder for the mock filesystem used in the unit tests for the rule because there was an edge case where ts.resolveModuleName could not correctly resolve aliases for folders that are directly at the root of the filesystem (it did not resolve /app/index.ts from @/app and same problem for all layers). After moving everything in /src, they resolve just fine. I thought about creating an issue in TS repository, but it seems like an extremely rare edge case that nobody would care about. No one puts their real projects in the root of the file system.