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

Emulate HMR bubbled argument to enable svelte-hmr CSS only updates #166

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rixo
Copy link

@rixo rixo commented Jan 30, 2021

This emulates the bubbled accept handler argument over Webpack HMR API, which allows svelte-hmr to detect CSS-only changes and skip recreating the whole component in those cases.

I'm not entirely confident in the correctness of the implementation... I suspect, in particular, that it could wrongly resolve bubbled to false in some cases, but to trigger those conditions the non-CSS code of the component would necessarily have changed, which means bubbled wouldn't be used by svelte-hmr anyway.

Still, this might require a bit of in-depth testing to ensure I haven't missed something, so I'm making a draft PR for now.

@non25 Can you help testing this change, and report if everything is in place and the fix works correctly?

@benmccann
Copy link
Member

CC @Smittyvb as well in case you'd like an extra pair of eyes for testing it

@non25
Copy link
Contributor

non25 commented Mar 4, 2021

I've been using this branch for like a month, haven't found something that would drive me off.
Sadly it can't successfully replace css when using svelte-pathfinder as a router.
Here's repro: https://github.com/non25/svelte-template-webpack/tree/test-hmr-bubbled

As to unrecoverable errors and stuff I feel like the situation is same, but the ability to do state-saving css reload was added, which is important.

@rixo what do you think, is there a possibility of regressions related to HMR with this ?
I don't even know how we would reliably test something like this.

@non25
Copy link
Contributor

non25 commented Mar 13, 2021

It seems that the approach of replacing just css doesn't work well with conditional markup.
Toggling conditional markup after HMR css update results in no css for that markup.
Will edit in a repro later.
Have to resort to preserveLocalState: true for current task, works better.

@rixo
Copy link
Author

rixo commented Mar 13, 2021

toggling conditional markup after HMR css update results in no css for that markup.

@non25 Funny you come up with that now. We identified the problem yesterday in the Vite plugin. It's because the compiler only apply the component scoping class to elements that it sees are actually matched by a selector. This means, when you add a selector CSS only updates are not enough.

Will have to reduce the cases where we do a CSS only update to fix that for now.

Eventually, we can search for a solution, probably with the help of the compiler. Maybe scoping everything in dev/hot would be the most straightforward, even if it's a bit concerning for HMR to change behavior so much as compared to prod.

Have to resort to preserveLocalState: true for current task, works better.

You do know of the magic comments that let you override this temporary at component level, without the need for restarting the dev server, right?

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