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.

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

Merged
merged 4 commits into from
Oct 31, 2024
Merged

Conversation

patak-dev
Copy link
Member

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.

@patak-dev patak-dev added this to the 6.0 milestone Oct 28, 2024
@bluwy
Copy link
Member

bluwy commented Oct 28, 2024

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.

@patak-dev
Copy link
Member Author

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.

@bluwy
Copy link
Member

bluwy commented Oct 28, 2024

I think if they were a deprecation message/warning, the feature should've still worked so it's not breaking, e.g build.polyfillModulePreload. If we're already breaking then the deprecation message probably isn't as useful. I would prefer removing it entirely though, especially given the option didn't have significant behavior difference on surface.

@patak-dev
Copy link
Member Author

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?

@sapphi-red
Copy link
Member

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.

@bluwy
Copy link
Member

bluwy commented Oct 29, 2024

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.

@sapphi-red
Copy link
Member

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.

@patak-dev patak-dev changed the title refactor: deprecate and make fs.cachedChecks noop refactor: remove fs.cachedChecks option Oct 30, 2024
@patak-dev patak-dev changed the title refactor: remove fs.cachedChecks option refactor!: remove fs.cachedChecks option Oct 30, 2024
@patak-dev
Copy link
Member Author

I removed the option as was decided in the team call today

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Thanks!

@bluwy bluwy merged commit 94b0857 into main Oct 31, 2024
15 checks passed
@bluwy bluwy deleted the refactor/remove-fs-cached-checks branch October 31, 2024 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants