sveltejs / sapper

The next small thing in web development, powered by Svelte
https://sapper.svelte.dev
MIT License
7k stars 434 forks source link

Better handling for 500: Failed to fetch #1724

Open AnandChowdhary opened 3 years ago

AnandChowdhary commented 3 years ago

Describe the bug This issue aims to be kind of an umbrella issue for this and similar problems that occur when a module is imported that doesn't exist and Sapper throws a 500.

Similar issues have been reported previously (e.g., https://github.com/sveltejs/sapper/issues/1144), but because they weren't adequately described, we couldn't work on them. Now that work is well under way on SvelteKit, I want to make sure that this is addressed there as well.

To Reproduce Say you have a Sapper website on production, you have the website open in your browser. Now, you deploy a new version of the website, meaning that module files have updated cache keys and therefore the old module paths would give 404 errors.

When you click on a link, say to the "About" page on the webpage that's already open (without refreshing it), Svelte tries to load the old module, say about.oldcache.js and throws a 500 error (Failed to fetch dynamically imported module). If you refresh the page and click on the about link again, Svelte will load about.newcache.js and everything works.

In https://github.com/sveltejs/sapper/issues/1144, @x4080 rightfully commented:

hi, maybe the error is related to cache, do you use serviceworker? i got this error and when refreshing the page error is gone

This is because refreshing the page actually loads the new, correct module.

Expected behavior No error is thrown and the webpage is correctly loaded. A 500 error also doesn't make sense because a 404 should be thrown if a module failed to load because the file path doesn't exist.

A potential solution would be: If there was an error in fetching the /about page (i.e., loading about.oldcache.js throws an exception), Svelte should use the default browser window.location.href redirect to take the user to /about instead of throwing 500 error. In case that webpage actually doesn't exist, we'll see the 404 error that, which is expected.

This solution should also be implemented in SvelteKit.

AnandChowdhary commented 3 years ago

I've added this pretty hacky fix for now and tested it a few times on production, it looks like it works well for me:

<script lang="ts">
  import { onMount } from "svelte";
  export let status: number;
  /**
   * Sometimes, we see that the chunk URL has changed and that
   * results in a 500 error (when we've deployed a new version)
   * In that case, simply refreshing the page works, so we do that
   * But we don't want to get stuck in a loop, so we only do it once
   */
  onMount(() => {
    if (typeof window !== "undefined" && "sessionStorage" in window && status !== 404) {
      if (!window.sessionStorage.getItem("sapper-has-refreshed")) {
        window.sessionStorage.setItem("sapper-has-refreshed", "1");
        window.location.reload();
      }
    }
  });
</script>

This idea is that we should refresh the page when we get an error; however, it could also be a "real" 500 error so we don't want to get stick in an infinite loop. This also points towards a potential solution being simply refreshing the page in case the module didn't load (since the address bar URL has been updated, refreshing the page will load the new module).

This is of course pretty hacky because we only refresh once even if we had a second deployment during the same session.

mquandalle commented 3 years ago

I think this is the same problem that the one discussed in #389. I agree that it should really be fixed in both Sapper and Svelte/kit !

mquandalle commented 3 years ago

Also @AnandChowdhary, I think there is a problem with your implementation because there is no expiry date on the local storage, which mean that the reload will only work for the first time and not on any further redeployments.

This could be fixed by storing the time of the sapper-has-refreshed token:

      const lastRefresh = window.sessionStorage.getItem("sapper-has-refreshed");
      if (!lastRefresh || lastRefresh.time < Date.now() - 1000 * 30) {
        window.sessionStorage.setItem("sapper-has-refreshed", Date.now());
        window.location.reload();
      }