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

HMR breaks context="module" exports #134

Closed
probablykasper opened this issue Aug 6, 2021 · 16 comments · Fixed by #437
Closed

HMR breaks context="module" exports #134

probablykasper opened this issue Aug 6, 2021 · 16 comments · Fixed by #437
Labels
bug Something isn't working hmr
Milestone

Comments

@probablykasper
Copy link

probablykasper commented Aug 6, 2021

Describe the bug

If you've exported a function using <script context="module">, HMR updates from that component causes other the exported function to no longer run from other components where it's imported.

Reproduction

https://github.com/probablykasper/svelte-vite-context-module-exports-bug

  1. npx degit probablykasper/svelte-vite-context-module-exports-bug svelte-vite-context-module-exports-bug
  2. cd svelte-vite-context-module-exports-bug && npm i
  3. npm run dev
  4. Open the website and check that the Show and Hide buttons work
  5. Trigger HMR in Counter.svelte, for example by changing the <h3> tag
  6. Now the Show and Hide buttons are broken

Logs

No response

System Info

System:
    OS: macOS 10.15.7
    CPU: (8) x64 Intel(R) Core(TM) i7-7700K CPU @ 4.20GHz
    Memory: 1.21 GB / 32.00 GB
    Shell: 3.2.2 - /usr/local/bin/fish
  Binaries:
    Node: 14.16.0 - /var/folders/44/3sxbyc2d6wg90mj1s7s59bnm0000gn/T/fnm_multishells/23267_1628209481847/bin/node
    Yarn: 1.22.10 - /var/folders/44/3sxbyc2d6wg90mj1s7s59bnm0000gn/T/fnm_multishells/23267_1628209481847/bin/yarn
    npm: 6.14.11 - /var/folders/44/3sxbyc2d6wg90mj1s7s59bnm0000gn/T/fnm_multishells/23267_1628209481847/bin/npm
  Browsers:
    Brave Browser: 92.1.27.108
    Chrome: 92.0.4515.131
    Firefox: 89.0.2
    Safari: 14.1.2
  npmPackages:
    @sveltejs/vite-plugin-svelte: 1.0.0-next.15 => 1.0.0-next.15 
    svelte: ^3.42.1 => 3.42.1 
    vite: ^2.4.4 => 2.4.4

Severity

annoyance

@probablykasper probablykasper added bug Something isn't working triage Awaiting triage by a project member labels Aug 6, 2021
@dominikg
Copy link
Member

dominikg commented Aug 6, 2021

broken code from repo above

App.svelte

<script lang="ts">
  import Counter, { visible } from './Counter.svelte'
</script>

<main>
  <Counter />
  <button on:click={() => visible.set(true)}>Show</button>
  <button on:click={() => visible.set(false)}>Hide</button>
</main>

Counter.svelte

<script lang="ts" context="module">
  import { writable } from 'svelte/store'

  let text = writable('')

  export const visible = writable(false)
</script>

<h3>Update me</h3>
<p>Visible: {$visible}</p>

what happens

When you edit Counter.svelte this results in a recompile and the js code is sent to the browser. svelte-hmr then "reapplies" that code and replaces all instances of Counter with new ones. The new Counters get a reference to a new instance of the visible store, as can be witnessed when clicking once so that the browser shows "visible true", and then editing Counter.svelte. It updates to "visible false", because it shows the initial value of the new instance.

The instance of App.svelte on the other hand remains unchanged and that imported the original instance of the visible store. So now you have 2 instances of the same store and of course updates to the one App.svelte has won't propagate to the new Counter.

how to avoid it

Move the store to it's own module to avoid recreating it on update

store.ts

import { writable } from 'svelte/store'
export const visible = writable(false)

App.svelte

<script lang="ts">
  import Counter from './Counter.svelte'
  import {visible} from "./store";
</script>

<main>
  <Counter />
  <button on:click={() => visible.set(true)}>Show</button>
  <button on:click={() => visible.set(false)}>Hide</button>
</main>

Counter.svelte

<script lang="ts">
    import {visible} from "./store";
</script>

<h3>Update bla</h3>
<p>Visible: {$visible}</p>

Now the hmr update of Counter no longer creates a new instance of the visible store and you can enjoy hmr.

cc @rixo

This is a limitation of the current implementation and trying to also invalidate all Components that import from an hmr updated Components context=module could cause a big cascade effect.

@dominikg dominikg added hmr and removed triage Awaiting triage by a project member labels Aug 6, 2021
@probablykasper
Copy link
Author

This is a limitation of the current implementation and trying to also invalidate all Components that import from an hmr updated Components context=module could cause a big cascade effect.

Could context="module" HMR updates work the same way as they do for JS modules?

If this is a lot of work to fix, a great temporary fix could be to fully reload if a component has exports in context="module"

@dominikg
Copy link
Member

dominikg commented Aug 6, 2021

a great temporary fix could be to fully reload if a component has exports in context="module"

unfortunately that would be pretty devastating for sveltekit which exports flags from context=module : https://kit.svelte.dev/docs#ssr-and-javascript-prerender

I'm chatting about it with @rixo and one approach would indeed be to split the current Counter.svelte into virtual Counter.module and Counter.component modules behind a Counter.svelte facade, with updates to Counter.module propagating to all importers via hmr bubbling.

But thats a pretty bold+risky move as the implementation involves creating 2 pretty jarring sourcemaps and at the end of the day results in more requests during dev.

Personal opinion:
The pattern i showed above where shared state is put into a separate module is not really a workaround but a better implementation.
context=module can be safely used encapsulate state between instances of a component or publish constants that never change. If it is to be modified from the outside, it should not reside in context=module.

So i think the best we can do for now is collect more Feedback/Ideas and document the limitation in the same way we documented the behavior around local state preservation.

https://github.com/sveltejs/vite-plugin-svelte/tree/main/packages/vite-plugin-svelte#why-is-component-state-reset-on-hmr-update

@probablykasper
Copy link
Author

The pattern i showed above where shared state is put into a separate module is not really a workaround but a better implementation.
context=module can be safely used encapsulate state between instances of a component or publish constants that never change. If it is to be modified from the outside, it should not reside in context=module.

I don't quite see why it's a better implementation, could you elaborate on it? In my case, I have a Create new modal. It only makes sense to have one instance of this modal, so I define visible = writable(false) in context="module" so that any component can check/update visible.

I considered both approaches and went for context="module", even knowing about this issue. visible is the state of this specific component, so imo it doesn't make sense to disconnect it, and I didn't really want to create a new file just for one boolean.

@probablykasper
Copy link
Author

unfortunately that would be pretty devastating for sveltekit which exports flags from context=module : https://kit.svelte.dev/docs#ssr-and-javascript-prerender

I don't understand why it affects SvelteKit. What if it does something equivalent, like an HMR update App.svelte?

@dominikg
Copy link
Member

dominikg commented Aug 7, 2021

If vite-plugin-svelte triggered a full-reload for every hmr update of a svelte component with exports from context=module that would lead to a very frustrating experience for sveltekit where these exports are a common occurence on page components.

The approach with the facade is worth investigating though as it could also lead to improvements for the case where you are only updating the instance script. I'm going to add a feature request for it to draft a solution and discuss possible issues with it

@probablykasper
Copy link
Author

Ah, I see. I assume checking for imports of context=module would work though

@Sayyiditow
Copy link

Hi guys, was this ever fixed or any good resolution?

@rixo
Copy link
Collaborator

rixo commented Sep 15, 2022

@Sayyiditow Hi! For now, the best workaround you can have is probably to split <script context=module> parts into their own .js file. I understand how this might not be practical in some situations, though.

The fix is in progress. Vite 3 has added the experimental option we need to address this on our side. I'm reserving some time to add it to svelte-hmr in the coming days, and hopefully it can make its way into the Vite plugin soon after.

FYI the fix effect will be that any change to the context=module part will cause invalidation of the whole .svelte SFC (single file component), meaning HMR will recreate all consumers (importers) of the SFC. If any of those consumers is not a Svelte component (or otherwise "accepted" HMR module), this will cause a full reload. Changes to the rest of the SFC (that is the Svelte component proper) will have HMR as usual. Edit I was out of my mind when writing this 😅 . What I described was the solution we did not implement because too brutal. The actual fix that has just been done is that only consumers of named exports (from context=module) get invalidated when the source module changes. Consumers of only the default export (that is, the Svelte component) do not get updated. For HMR this is the equivalent to have the part in context=module in a separate .js file -- and this is the best than can be handled from within Svelte.

@Sayyiditow
Copy link

@rixo Thanks for the update. We have some work around for now.

dominikg added a commit that referenced this issue Sep 17, 2022
dominikg added a commit that referenced this issue Sep 17, 2022
* fix: update svelte-hmr and enable partial accept to allow context=module updates, see issue #134

* test: harden partialAccept test (#441)

* refactor: improve test logic to avoid jest syntax from hell

Co-authored-by: rixo <rixo@rixo.fr>
@dominikg
Copy link
Member

should be fixed in 1.0.6

@Sayyiditow
Copy link

Tried 1.0.6 but still getting the same error when using svelte-materialify components such as that have context=module.

Anyone who can report that the fix works?

@probablykasper
Copy link
Author

It seems to work for me

@dominikg
Copy link
Member

dominikg commented Sep 20, 2022

@Sayyiditow if you still face this issue with @sveltejs/vite-plugin-svelte v 1.0.6, and svelte-hmr 0.15, please provide a link to a repo that reproduces it and instructions how excactly to trigger.

@jdkdev
Copy link

jdkdev commented Oct 26, 2022

Hey @dominikg ,

I just tried upgrading to Vite 3 with a svelte/routify project and am using:

"vite": "^3.1.8",
"svelte": "^3.52.0",
"svelte-hmr": "^0.15.0"
 "@sveltejs/vite-plugin-svelte": "^1.0.9",

and getting these errors:

✘ [ERROR] Ambiguous import "merge" has multiple matching exports

    script:/web/src/pages/emails/_emails.Previewer.svelte?id=1:2:11:
      2 │   import { merge, views } from '@/resources/Template.svelte'
        ╵            ~~~~~

  One matching export is here:

    script:/web/src/resources/Template.svelte?id=0:7:26:
      7 │   export { service, make, merge, views }
        ╵                           ~~~~~

  Another matching export is here:

    script:/web/src/resources/Template.svelte?id=1:7:26:
      7 │   export { service, make, merge, views }
        ╵                           ~~~~~

These exports are in a <script type="module">.

Does this seem like the bug in this ticket, or something else?

@dominikg
Copy link
Member

the fix for this (partial HMR) might have uncovered this. Can't tell if this is a bug in v-p-s / svelte-hmr or a problem in your project.

Please file a new issue and provide a minimal reproduction. Ideally without routify in a clean vite+svelte project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working hmr
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants