fix: ignore change/addition in empty view/layout/index files #2297
Conversation
Hilla's FS Router vite-plugin watches for the change, addition, or removal of the files under `views` directory, and determines the FS router configurations based on the updates. When a new file is added anywhere under views it can be empty or doesn't export any components or route configurations. In this case, that file should be ignored. Fixes #2204
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2297 +/- ##
==========================================
+ Coverage 95.22% 95.24% +0.02%
==========================================
Files 66 66
Lines 4292 4292
Branches 611 612 +1
==========================================
+ Hits 4087 4088 +1
+ Misses 163 162 -1
Partials 42 42
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -74,6 +75,10 @@ export default async function collectRoutesFromFS( | |||
| children.push(await collectRoutesFromFS(new URL(`${d.name}/`, dir), { extensions, logger, parent: dir })); | |||
| } else if (d.isFile() && extensions.includes(extname(d.name))) { | |||
| const file = new URL(d.name, dir); | |||
| const url = await checkFile(file, logger); | |||
There was a problem hiding this comment.
That's definitely not the best place to check files because there is a lot of files we're skipping. That's why I was checking the file in the array after that.
There was a problem hiding this comment.
That's definitely not the best place to check files because there is a lot of files we're skipping. That's why I was checking the file in the array after that.
Could you please elaborate more about the files we are skipping if we do the checking here?
To my understanding, this branch of else if is the one adding the entires to the children and we only care about those files with the specified extensions, so I couldn't understand what are the skipped files, and why is it important to first add some entires to children for empty files (or those that are not exporting a component) and then filter them later.
A brief description of the rule in my mind:
Files with .tsx or .jsx extension that are empty or doesn't have a export default (yet) can be considered non-existent from the FS Router POV. No need to consider their existence in the first place.
There was a problem hiding this comment.
The idea behind my comment was that we're skipping the files that start with _. However, in your implementation, they are also checked. However, each check is IO operation which is expensive. That's why I perform the check only with already added files.
Also, for me, it looks more logically correct when all checks are done in the same place somehow.
There was a problem hiding this comment.
Well, TBH, I'm also a fan of keeping those file checking in one place, so that it is easier to located it in the code, or even extract the whole thing as a function. That's why I tried adding the filter before the map operation, but I couldn't get it to work. There's a chance that I missed to put the correct combination of async-await while filtering, but I'd say if it is going to be a very complicated combination of nested await Promise.all..., I'd simply go for the current less complicated implementation.
| layout: l, | ||
| }; | ||
| }), | ||
| children.map(async (child) => ({ |
There was a problem hiding this comment.
What if instead of removing the filecheck here, we filter the array according to the results of checkFile?
There was a problem hiding this comment.
My reason for removing the checkFile call for the file was due to the fact that it's done earlier before adding it to the children entries. In case we want to add filtering to the pipeline before the map, we still don't need to call the checkFile on file as it is done earlier, unless we don't check the file earlier.
|
|
This ticket/PR has been released with Hilla 24.4.0.alpha21 and is also targeting the upcoming stable 24.4.0 version. |
Description
FS Router's vite-plugin watches for the change, addition, or removal of the files under
viewsdirectory, and determines the FS router configurations based on the updates. When a new file is added anywhere under views it can be empty or doesn't export any components or route configurations. In this case, that file should be ignored.Fixes #2204
Type of change
Checklist