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(client): simplify fetchUpdate code #11004

Merged
merged 5 commits into from
Nov 22, 2022
Merged

refactor(client): simplify fetchUpdate code #11004

merged 5 commits into from
Nov 22, 2022

Conversation

ArnaudBarre
Copy link
Member

@ArnaudBarre ArnaudBarre commented Nov 20, 2022

I spend few hours understanding the logic of the client and wanted to cleanup some inconsistency before sending a PR to update the hot.accept API.

Each commit is independant.

The main fix to me is the drop of the moduleMap that introduced me in error at the beginning. Thanks @patak-dev for this comment!

Re: #7475 (comment)

This is technically breaking is people when doing calling noop decline method, but that's a good thing we're in alpha!

@patak-dev
Copy link
Member

Thanks for the step-by-step process @ArnaudBarre!

There is a message here that says hot.decline() was meant to be used for static analysis 🤔 1c12e26#diff-dc569afbb42d4c723c76a7f70f5153a65f73c26e0ad350bd3cb5934367f8f615R329. It would be interesting to get more info about this removal.

I think we should extract 9db55db (remove hot.decline) and 92fef6c (expose hot.prune) into other two PRs so we can squash and merge this one, and leave a proper trace of the changes to the hot API.

@ArnaudBarre
Copy link
Member Author

I don't know what was the initial goal for decline, but invalidate becoming a lot more powerful that what it was during this commit, I think it was never implemented or removed.

I grep decline in the monorepo and I removed all the usage, which is just a noop function and the documentation.

Plus if people want to trigger a location reload, they can just directly use location.reload()

I will split prune into another PR and decline into another one if you agree to drop it

@patak-dev
Copy link
Member

I think we should have both PRs, and we'll check quickly with Evan just in case pointing to them. I also agree that having this noop isn't needed.

@patak-dev patak-dev changed the title chore(client)!: Add prune to types and remove never implemented hot.decline chore(client)!: remove never implemented hot.decline Nov 22, 2022
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.

The refactors looks great to me. Re decline(), I think we should still keep it to prevent a breaking change. Astro is currently using it.

It's also part of the spec so it might be a good idea to keep it for now to be compliant. And in case we ever find a use for it.

@patak-dev
Copy link
Member

@bluwy maybe you could check why Astro is using it? It isn't doing anything on Vite's side. Maybe they believe it is working and it is hiding bugs (probably it was implemented in Snowpack). @ArnaudBarre we should split hot.decline as we discussed so we can merge the refactorings in this PR, and we can keep discussing the future of hot.decline in the other PR. At least we should document that it is a noop if we plan to keep it?

@ArnaudBarre
Copy link
Member Author

I will do that.

In case we need to keep for backward compatibility, I would just keep it in the JS runtime but removes it from the type so that meta framework can be warn without breaking code from end users

@ArnaudBarre ArnaudBarre changed the title chore(client)!: remove never implemented hot.decline chore(client): simplify fetchUpdate code Nov 22, 2022
@patak-dev patak-dev requested a review from bluwy November 22, 2022 16:37
@bluwy bluwy changed the title chore(client): simplify fetchUpdate code refactor(client): simplify fetchUpdate code Nov 22, 2022
@patak-dev patak-dev merged commit f777b55 into vitejs:main Nov 22, 2022
@ArnaudBarre ArnaudBarre deleted the chore-client branch November 22, 2022 21:17
fc pushed a commit to fc/vite that referenced this pull request Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants