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

chore(client)!: remove never implemented hot.decline #11036

Merged
merged 1 commit into from Dec 3, 2022
Merged

chore(client)!: remove never implemented hot.decline #11036

merged 1 commit into from Dec 3, 2022

Conversation

ArnaudBarre
Copy link
Member

Re: #11004

@patak-dev patak-dev added feat: hmr p1-chore Doesn't change code behavior (priority) labels Nov 22, 2022
@bluwy
Copy link
Member

bluwy commented Nov 22, 2022

Re #11004 (comment)

@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).

Yeah it's likely a remnant from Snowpack HMR days, but I don't think we should break it. It still has an intent and is future-proof, e.g. if one day Astro decides to use a different framework that implements .decline().

I think we should document it and type it still, but maybe put a note that it's currently a no-op.

@patak-dev
Copy link
Member

patak-dev commented Nov 22, 2022

Agree that we could leave it if there was a good reason for it. But I wonder why Vite doesn't need to implement it.

But does it makes sense that Astro is using it? It looks hot.decline was added by @natemoo-re only a few months ago withastro/astro#3932

@ArnaudBarre
Copy link
Member Author

I didn't see any clear intent for it.
Either:

  • you know ahead of time HMR is not possible and just don't append anything to the file
  • you know at runtime you need to invalidate and call hot.invalidate
  • you need to undo some side effects and use hot.dispose
  • you know the only possible way to recover is to reload the page and call location.reload

The last point was probably the intended use, but adding an entry to an API that is just an alias for a well know supported platform feature feels wrong.

If another need appear in the feature, I think it's better to discuss it and find a new name in the API for it

@patak-dev
Copy link
Member

@FredKSchott maybe you could shed some light on why hot.decline was added to the API? Maybe it is there for historic reasons. It could have been added first, then invalidate was added later and made the former a bit redundant.

@natemoo-re
Copy link
Contributor

But does it makes sense that Astro is using it? It looks hot.decline was added by @natemoo-re only a few months ago withastro/astro#3932

Probably not! I was just throwing things at the wall trying to fix some HMR stuff. If it's a noop this can be removed.

@FredKSchott
Copy link
Contributor

Yea, there's a lot of history here from the early Snowpack and Vite days. You can see the early repo where Evan and I were exploring implementing the same API: https://github.com/FredKSchott/esm-hmr

You can see this API spec'd out there: https://github.com/FredKSchott/esm-hmr#decline

It's been long enough that I don't remember the history of why this was needed, but I have no need to keep it around at this point if its a no-op. I'm overdue to add a note to that repo README that the idea of a shared "spec" is abandoned, and to redirect people to Vite's HMR docs as the most likely up-to-date docs.

@bluwy
Copy link
Member

bluwy commented Nov 23, 2022

I still feel like we shouldn't remove it just because we don't implement it though. Webpack also implements decline and perhaps we should implement the full reload if it's not working now.

If another need appear in the feature, I think it's better to discuss it and find a new name in the API for it

I don't think we should create a new API name when there's prior art.


I'm maybe in the outlier here 😬 But I think we should avoid breaking things unless there's a compelling reason besides cleaning things up.

@patak-dev
Copy link
Member

I think the fact that webpack implements it is a good point in favor of keeping hot.decline. FWIW, I thought the idea of removing it was good not as a clean-up but to avoid confusing users. Because currently, as it happened to Nate, they may think that the API is actually having an effect. We could leave it with the justification of backward compat and document that this API is a noop in Vite, and point to what should be used instead to achieve the same.

@ArnaudBarre
Copy link
Member Author

For me it should at least be remove from types, because people will read their autocomplete before reading the docs and it can takes sometimes before seeing that it does nothing and go reading the doc

@bluwy
Copy link
Member

bluwy commented Nov 23, 2022

Removing from the types sounds like a nice middle ground for me too. And also updating the docs regarding it being a no-op and could be implemented in the future, plus directing what to use instead.

@ArnaudBarre
Copy link
Member Author

@bluwy @patak-dev Are you ok with this version?

@patak-dev patak-dev merged commit e257e3b into vitejs:main Dec 3, 2022
@ArnaudBarre ArnaudBarre deleted the drop-hot-decline branch December 3, 2022 21:18
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.

Looks good to me 👍

futurGH pushed a commit to futurGH/vite that referenced this pull request Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: hmr p1-chore Doesn't change code behavior (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants