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

fix: reload in case of missing chunk ( e.g. site updated) #3940

Merged
merged 9 commits into from
Dec 19, 2018

Conversation

aldarund
Copy link

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

Resolves: #3389
Simplest solution for now, just reload window with current path in case of loading chunk error. Suggestions how to handle better welcome :)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly. (PR: #)
  • I have added tests to cover my changes (if not applicable, please state why)
  • All new and existing tests passed.

@codecov-io
Copy link

codecov-io commented Sep 22, 2018

Codecov Report

Merging #3940 into dev will increase coverage by 15.76%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##              dev    #3940       +/-   ##
===========================================
+ Coverage   81.67%   97.43%   +15.76%     
===========================================
  Files          29       18       -11     
  Lines        1566     1248      -318     
  Branches      409      345       -64     
===========================================
- Hits         1279     1216       -63     
+ Misses        227       31      -196     
+ Partials       60        1       -59
Impacted Files Coverage Δ
packages/builder/src/webpack/plugins/vue/client.js
packages/cli/src/index.js
packages/common/src/utils.js
packages/core/src/resolver.js
packages/builder/src/webpack/utils/perf-loader.js
packages/cli/src/common/utils.js
packages/core/src/meta.js
packages/cli/src/commands/start.js
packages/cli/src/commands/generate.js
packages/cli/bin/nuxt.js
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ec5cda...951c601. Read the comment docs.

@@ -140,6 +140,9 @@ async function loadAsyncComponents (to, from, next) {
next()
} catch (err) {
err = err || {}
if (/^Loading chunk (\d)+ failed\./.test(err.message) && process.client) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is process.client necessary here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really as i understand, was just trying to be super safe

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, we are already in app/client.js

@@ -140,6 +140,9 @@ async function loadAsyncComponents (to, from, next) {
next()
} catch (err) {
err = err || {}
if (/^Loading chunk (\d)+ failed\./.test(err.message) && process.client) {
window.location.replace(location.href)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this improvement, we may make behavior more configurable. Sometimes persisting store data is needed for example before redirect or something like that.

My samplet suggestion is something like scroll behavior. But serialization is not actually a flexible idea. We can think about possible ways.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ye, making it configurable sounds fine. I could make it same way as scrollBehaviour if there no better idea.

@Atinux
Copy link
Member

Atinux commented Sep 23, 2018

Actually, I believe this could be the default behaviour for Nuxt to avoid displaying error.

One idea is to call the errorHandler before redirecting or some kind of hook so user could take control of it instead of reloading. WDYT?

@aldarund
Copy link
Author

Yes, some kind of control\override would be good. One way would be same way as with scrollbehaviour with serailizing function. Is there examples of similar things with hooks?

@pimlie
Copy link

pimlie commented Sep 23, 2018

Isnt this behaviour quite confusing for a visitor? The reloading seems only a valid solution for when a new build with new hashes has been published and all chunks are really available. But what if for some reason the chunk just really doesnt exist?

Eg if you generate your nuxt project and then manually upload the generated files to a webserver, but one of the chunks 'fails' to upload. Reloading the page will not be a solution then. I think it will be confusing for visitors that when they click a link the page just reloads the current page without displaying an error message.
Actually, in case a project was generated it would be much better to just reload to the new url instead of the old url as possibly the generated html file does still exist.

@pi0
Copy link
Member

pi0 commented Sep 23, 2018

Thanks to the note of @pimlie 😃👌 We may need to add a query parameter to the page when reloading (/foo?chunkError) or check to not reload if referrer is the same page. If chunk doesn't exist or net is down window will be infinity reloaded.

@pimlie
Copy link

pimlie commented Sep 24, 2018

What about a nuxt.config.js option and/or a build parameter to keep the chunk files for the last X number of builds? Currently the dist folders (.nuxt/dist and dist/_nuxt) are indiscriminately removed when building or generating, but maybe we should add support to keep the chunks so we dont break 'running' apps in between site updates. Maybe even also add a method for indicating the chunk is still 'fresh' or out of date (probably a bit difficult to do this fully agnostic but i.e. a replaceable boolean string in the chunk files), so we can either show a message like 'a new version of this page is available, please reload' or just reload fully automatically on the next navigation instead.

This way we 'intelligently' fix the issue of missing chunks that we caused ourselves by updating our site without breaking the user-experience. And then we also know that in other cases its most probably best to really display a temporary or hard error.

--edit--
Or just replace the old chunks with a stub that does the page reload for us

@aldarund
Copy link
Author

@pimlie keeping old files is a different issue and it wont solve this problem at all. There issue opened for this. Keeping old files implies that build are done using same storage every time. Usual workflow is building in new environment on CI, so there wont be old files unless some special setup where u store save old files somewhere, and download them on next build.

@pimlie
Copy link

pimlie commented Sep 24, 2018

@aldarund Maybe I am a bit slow on this monday, but why wouldnt my suggestion solve the problem at all?

It seems to me that if you break down the issue this PR tries to fix there are three main possible reasons why an app running in a users browser cant find a chunk:

  1. it doesnt exists anymore because a new version has been published
  2. it never existed
  3. there is a temporary lookup failure (eg network)

So why not try to handle each case the best we can while we are at it?

@aldarund
Copy link
Author

@pimlie ok, not at all, but the problem will still happen with your solution when deployments happen without previous sources, which is quite common case that certainly would happen . For what u are talking there is issues like this -> #3078
This pr intended to fix situation 1 ( and maybe 3) , without going too deep, because proper solution will require a lot more things.

@aldarund aldarund added the WIP label Sep 25, 2018
@husayt
Copy link
Contributor

husayt commented Sep 28, 2018

This is probably most important outstanding fix at the moment for Nuxt. As this issue is the only one I know which causes broken page for everyone on new deploy. Just need to make sure we don;t get infinite reload problem.

Thanks @aldarund

@pi0
Copy link
Member

pi0 commented Sep 28, 2018

Or just replace the old chunks with a stub that does the page reload for us

This would be possible using a connect middleware to respond for all 404 requests matching /_nuxt/*.js with a window.reload. But it won't work for CDN deployments.

pi0 and others added 4 commits September 28, 2018 12:22
- ensure message is a string
- use location.reload to support hash # and also explictly bypass cache
- added minor comments
@aldarund
Copy link
Author

@husayt i have tested and was unable to get infinite reload. If page loaded directly ( e.g. via reload or direct navigation) it will throw app initialization error in console https://i.imgur.com/CWAFfFC.png ( thats current behaviour even without this PR )

@usulix
Copy link

usulix commented Oct 11, 2018

As a senior developer on a team with 2 current production Nuxt apps, I wanted to chime in here.

first - Thanks for the great framework and for the hard work. I am working to get corporate to fund some support for the project :-)

I think @aldarund has a simple and workable fix, and I put it in my error.php file to handle the situation, but beyond that... when you ask the infinite reload question, for me it becomes an end-user question. It depends a lot on hosting and even more on deployment process. It also depends on what I want my end-users to experience.

For example, I am thinking about dropping a counter in local storage because I want to try the reload a few times... flashing an "App is updating... please wait" message... but then giving up with "App update failed. Please try again later." if the count is too high... But, that is my solution based upon the user experience for my app...

My personal opinion is that a Nuxt internals fix for this might be the wrong path, because I'm not sure the project can completely anticipate all the crazy things I might do as an end user nor all the jacked up ways I might deploy. Maybe this is a place where documentation needs to be clear and implementation is left up to the user.

just my 2 cents...

@aldarund
Copy link
Author

aldarund commented Oct 11, 2018

@usulix well, there wont be case when it will be infinite reload. If such errors happens and reload happens - app will fail initialiaze and no more logic will be working at all ( it will be vue initialize error and none of vue js logic will work)

@usulix
Copy link

usulix commented Oct 11, 2018

@aldarund , thanks for the education on Discord... checks in every way I tried to emulate.. first chunk failed error in loaded app.. gets redirect.. on page refresh, I get an app load error and no infinite redirect.

@fuzzybaird
Copy link

I would love to see this one through to dev :) Its a real problem for us in Prod and this implementation looks like it would be a reliable fix.

@axetroy
Copy link

axetroy commented Oct 18, 2018

How about reloading the manifest and re-get the page again?

@ahus1
Copy link

ahus1 commented Oct 20, 2018

I've posted a cut-and-paste workaround with a reload-once-strategy to #3389 that works by adding it to the error page template (no internals need to be changed) - maybe this helps some of the users who commented here.

@pi0
Copy link
Member

pi0 commented Oct 20, 2018

@ahus1 I exactly thought about that. But there is a potential pitfall that using custom error layouts, users have to provide their own missing chunk handler solution.

@ahus1
Copy link

ahus1 commented Oct 20, 2018

@pi0 the workaround is only a workaround, but AFAIK this is what is possible with the versions released today. I am really looking forward for this PR to be completed, merged and released. Then the workaround from the error layout can be removed again.

@stale
Copy link

stale bot commented Nov 15, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Nov 15, 2018
@manniL
Copy link
Member

manniL commented Nov 17, 2018

Should we still implement this after #4323? 🤔

@stale stale bot removed the stale label Nov 17, 2018
@pi0
Copy link
Member

pi0 commented Nov 17, 2018

@manniL While #4323 removed perf and runtime issues still missing chunks after a reload happen and such workaround may be added as a built-in functionality to auto-reload or gracefully handle this error.

@manniL
Copy link
Member

manniL commented Nov 17, 2018

@pi0 Agreed 👍

@fuzzybaird
Copy link

I agree with @pi0, #4323 looks like it does not exactly address this issue. I guess where I am confused is why this implementation of #3940 is bad specifically? I have had over 30 failed chunks on production from deploying to our CDN since this PR was proposed. I would be happy to come up with an alternate solution to this if someone can guide me as to what they think a better path would be.

@manniL
Copy link
Member

manniL commented Dec 5, 2018

What's the status? ☺️

@aldarund
Copy link
Author

aldarund commented Dec 6, 2018

@pi0 @manniL so do we go to for this simple solution or add more things like allow to override its behaviour etc?

@manniL
Copy link
Member

manniL commented Dec 6, 2018

I'm okay with using the "simple" approach for now as it'll resolve the problems. We can later provide a customizable option if needed.

@main2018
Copy link

main2018 commented Jan 4, 2019

What should I do, just upgrade nuxt?

@manniL
Copy link
Member

manniL commented Jan 4, 2019

@main2018 Not released yet, so either wait for 2.4 or use nuxt-edge (uninstall nuxt, install nuxt-edge, that's it :) )

@main2018
Copy link

main2018 commented Jan 4, 2019

@manniL 3Q very much indeed,This problem has been bothering me for several days.

@sskyu sskyu mentioned this pull request Mar 7, 2019
7 tasks
@A-Tione
Copy link

A-Tione commented Feb 4, 2021

@main2018 Not released yet, so either wait for 2.4 or use nuxt-edge (uninstall nuxt, install nuxt-edge, that's it :) )

ecently, I have also encountered this problem. Have you solved it now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[edge] Deployment of new version breaking site for live users. ( Loading chunk {n} failed error)