-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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.
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
refactor!: remove fs.cachedChecks option #18493
Conversation
If it's a noop, wouldn't it be a stone's throw away to directly remove the option too? I imagine it only affects the types, or if someone is basing that config to do something different, but both seems not very breaking to me. |
It only affects the types. I kept it because it may be helpful for a bit to have the deprecation warning instead of the type error. We could remove it from the config docs if you think that is a good idea. Or fine with me if you want to remove it all, there shouldn't be that many users setting it. |
I think if they were a deprecation message/warning, the feature should've still worked so it's not breaking, e.g |
Making it noop isn't breaking though. This was a transparent optimization. @sapphi-red recommended to make it a noop too. Sapphi maybe you have better reasons to explain why noop is better here? |
Even if this does not affect many users, it's still good to reduce breaking changes. Because if it's a breaking change, users will still need to check and judge whether that change that affects them. Deprecations can be checked after migration. Keeping this flag in the types does not cost us much, so I think it's worth to leave the flag. |
I don't know if I agree with that. We're already making greater breaking changes elsewhere. Removing a property from the types where the feature itself is already removed and not common for people to configure it anyways feels like the least concern of breaking changes to anyone. You're not getting any runtime regressions either, it's only the types. |
I agree that it is easy to remove the property. It just makes the migration slightly easier as the users won't even need to notice about this. It's a small thing so I'm fine with removing it completely. |
I removed the option as was decided in the team call today |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Description
We introduced this feature to optimize fs resolving at #15279. Later on we enabled it by default, but we had to backtrack and make it opt-in at #17807 due to edge cases in HMR when adding new files. I don't think we're going to be able to make this optimization reliable at the end, and we may move to Oxc resolver instead anyways.
This PR makes
fs.cachedChecks
a noop for Vite 6, and deprecates the option so we don't need to keep maintaining this optimization if we aren't going to be recommending it to users.