EmerisHQ / demeris

Emeris web app
https://app.emeris.com/
Apache License 2.0
12 stars 2 forks source link

Force reload demeris when errors are thrown due to probable new build. #1715

Open fl-y opened 2 years ago

fl-y commented 2 years ago

Describe the bug Sentry is flooded with the following error Failed to fetch dynamically imported module The error was effectively reproduced in this thread. Since said error is an effective indicator that the user is using an outdated build of Emeris, Adding client code s.t. Demeris is force-reloaded from the client side when said error occurs will greatly reduce those generic errors.

To Reproduce Keep Emeris open while a PR is being merged to PROD, give it 5~10 minutes and try navigating to different pages.

Expected behavior Instead of a malfunctional website throwing errors - the client should force-refresh itself.

Additional action points This is not a long term solution but effective duct-tape for the current state of things. In the long run a new(breaking) build should trigger an alert / message to any active website sessions and trigger a refresh.

Implementation

Message: A new version of the website was detected! The page will now refresh. ^^ Wdyt? @josietyleung

josietyleung commented 2 years ago

Thanks for the initiative! Fix sounds great, let's do this πŸ™

Dawntraoz commented 2 years ago

Maybe we can try some Vite configurations for the chunks: https://vitejs.dev/guide/build.html#chunking-strategy

Same issue but with Webpack: https://stackoverflow.com/questions/54889720/handle-loading-chunk-failed-errors-with-lazy-loading-code-splitting

Dawntraoz commented 2 years ago

@clockworkgr has found this article for a workaround https://www.google.com/url?sa=t&source=web&rct=j&url=https://medium.com/%40codemonk/automatic-reload-of-client-after-deploying-in-vue-js-91c120f85f0e&ved=2ahUKEwi67uzR3cP3AhWVg_0HHab6D5sQFnoECAgQAQ&usg=AOvVaw1gqzlz8D0bCaDmSX5_LYZ0

Dawntraoz commented 2 years ago

This also explain the issue in detail and propose to do the versioning (not deleting the old version, so the user avoid this problem before refreshing): https://rollbar.com/blog/javascript-chunk-load-error/

fl-y commented 2 years ago

Considering all the talk we went through regarding this topic, and how @eitjuh 's PR was closed to unknown reasons, Will work on this with 'minimum effort & instant fire-fighting mindset'.

Wrote up something but realized it was pretty much what this issue was suggesting as the fix anyway πŸ˜…

luciorubeens commented 2 years ago

Debugging a bit, I noticed that Vite uses the following approach to load new modules:

https://user-images.githubusercontent.com/4539235/168497416-3b7833d2-c65d-4421-ad7d-2ea3ad166a7f.mp4

TL;DR: Vite inserts a new <link rel="modulepreload"> tag with the target file when the user navigates.

So a possible solution would be to preload all the module files at start, I found this plugin that does exactly what I'm expecting:

https://github.com/jarlef/vite-plugin-preload

The output will result in this:

CleanShot 2022-05-15 at 20 21 43

Dawntraoz commented 2 years ago

Debugging a bit, I noticed that Vite uses the following approach to load new modules:

CleanShot.2022-05-15.at.19.52.36.mp4 TL;DR: Vite inserts a new <link rel="modulepreload"> tag with the target file when the user navigates.

So a possible solution would be to preload all the module files at start, I found this plugin that does exactly what I'm expecting:

https://github.com/jarlef/vite-plugin-preload

The output will result in this:

CleanShot 2022-05-15 at 20 21 43

@luciorubeens Exactly, that is how it should work for lazy loading the modules while needed, and for that we are doing dynamic import and not loading everything at once. If you modulepreload everything in one place (the first load) then the performance will be worse than it's now.

We can try this change locally and measure the performance in both cases, to see if it's a number of ms or s we can allow, or it's too much.

luciorubeens commented 2 years ago

Ok, here we go:

Preloading all files:

CleanShot 2022-05-16 at 08 42 46@2x

Current:

CleanShot 2022-05-16 at 08 40 06@2x

So i think it's discarded πŸ˜…

luciorubeens commented 2 years ago

Ok, another option would be to not clear the build files on each deploy, then dist will start to look like this:

CleanShot 2022-05-16 at 08 53 38

Here is the option: https://vitejs.dev/config/#build-emptyoutdir

Will require a script, workflow or something like to remove old files.

pranaybaldev commented 2 years ago

Just adding context for the downvote... I think there's a considerable difference in the core web vitals for the two cases and I would prefer not going for the worse case :)

eitjuh commented 2 years ago

Ok, another option would be to not clear the build files on each deploy, then dist will start to look like this:

CleanShot 2022-05-16 at 08 53 38

Here is the option: vitejs.dev/config/#build-emptyoutdir

Will require a script, workflow or something like to remove old files.

@luciorubeens sounds like a good option πŸ‘ , let's do it?

caudurodev commented 2 years ago

What about listening for the error event and requesting to the user to reload if it occurs? A message like: your page is outdated - click to reload before proceeding:

<script type="text/javascript">
  window.onerror = (msg, url, line, col, error) => {
    if (
      msg === 'SyntaxError' &&
      error === "Unexpected token '<'" &&
      !window.location.hash
    ) {
// ask nicely before reloading...
      window.location = window.location + '#refresh'
      window.location.reload()
    }
  }
</script>

other ideas here: https://imrecsige.dev/garden/how-to-solve-missing-chunk-code-splitting-errors-after-deploy/

luciorubeens commented 2 years ago

@caudurodev Yes, exactly what @fl-y is doing here #1802. Not a fan of this option because the user will lose state, like pending transactions. We can also persist this in localStorage and rehydrate on load to fix this edge case, but..

CleanShot 2022-05-16 at 11 17 23

Thanks for sharing the link. So any option has its cost, so we need to choose the one that works best for us.

caudurodev commented 2 years ago

I think we won't have a solution that will be good to preserve state as it is currently stored impermanently in memory.

How will you preserve state if there have been changes to the store for example? It seems like a minefield of edge cases.

If we really want to maintain state, the easiest option would be via a backend session and another, turning our site into a PWA and persisting data in localstorage/sessions.

There are some packages that help with persisting Vuex like https://www.npmjs.com/package/vuex-persist

But just the sheer amount of work to get this to work and all the edge cases... what if the user reloads mid-transaction? What if...

eitjuh commented 2 years ago

@caudurodev fair concerns! However, I find

What about listening for the error event and requesting to the user to reload if it occurs? unacceptable as well, because it doesn't scale well. We will be doing many many deploys per day - let's say 100. Then if you keep your Emeris open for an hour, you're asked 4 times to reload. We can't bother the customer with that.

Agree with the minefield of edge cases - but on the other hand, my guesstimate is that with most of the changes - the asset actually didn't change at all (most often a deploy doesn't necessarily touch the files you're using). (and even IF the user is using a specific file, I think in most cases it would be harmless to update it) - however, those are my guesstimates which are not worth that much.