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(vue-app): prevent calling asyncData in keep-alive pages (#5945) #5947

Closed
wants to merge 6 commits into from
Closed

Conversation

Elevista
Copy link

@Elevista Elevista commented Jun 17, 2019

Types of changes

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

Description

Resolves: #5945
Reproduction link: https://codesandbox.io/embed/codesandbox-nuxt-7ostd

related code
https://github.com/nuxt/nuxt.js/blob/f3d29e378087b6c6c686a03292c2bce409161110/packages/vue-app/template/client.js#L386-L397
https://github.com/nuxt/nuxt.js/blob/f3d29e378087b6c6c686a03292c2bce409161110/packages/vue-app/template/utils.js#L24-L31
Component.options.__hasNuxtData value of SSR rendered keep-alive page is true

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 are passing.

@Atinux
Copy link
Member

Atinux commented Jun 17, 2019

Thanks for the PR.

A test is failing and actually it should not since it actually does not use keep-alive (https://github.com/nuxt/nuxt.js/blob/dev/test/fixtures/children/layouts/patch.vue#L3).

In order to check it, I believe you may want to keep the instance of the page, if it exists, get its VNode and checks if it uses keepAlive (see https://github.com/vuejs/vue-router/blob/dev/src/components/view.js#L34)

@Elevista
Copy link
Author

Elevista commented Jun 18, 2019

@Atinux oh yeah... I forgot e2e test 😨
I'll figure this out

p.s. I had checked test:fixtures and all passed.
test:e2e had to run individually..

@codecov-io
Copy link

codecov-io commented Jun 18, 2019

Codecov Report

Merging #5947 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #5947   +/-   ##
=======================================
  Coverage   95.68%   95.68%           
=======================================
  Files          82       82           
  Lines        2689     2689           
  Branches      690      690           
=======================================
  Hits         2573     2573           
  Misses         98       98           
  Partials       18       18
Flag Coverage Δ
#e2e 100% <ø> (ø) ⬆️
#fixtures 50.35% <ø> (ø) ⬆️
#unit 92.67% <ø> (ø) ⬆️

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 ac723e6...f6ef962. Read the comment docs.

@Atinux Atinux self-requested a review June 24, 2019 13:09
@Atinux
Copy link
Member

Atinux commented Jun 24, 2019

Nice @Elevista

Would be nice to complete with another test (I guess adding a new layout with <keep-alive>, something like patch-keep-alive.vue there: https://github.com/nuxt/nuxt.js/blob/dev/test/fixtures/children/layouts/)

@leebyungjun-nbt
Copy link

@Atinux
Ok, I'll try

@Elevista
Copy link
Author

little late because I'm busy with work these days. 😅

@manniL manniL requested a review from pi0 July 20, 2019 18:51
@highruned
Copy link

highruned commented Jul 22, 2019

Why shouldn't asyncData be called again with keep-alive? data is called. What makes asyncData different? it's often used as a replacement simply to make it SSR compatible. I use the params.id to fetch different model data in asyncData. When navigating, the function is called, but it's not updating the UI, because it's missing the same workaround that data has..
https://github.com/nuxt/nuxt.js/blob/055e99a21b0c2f681025c5adc90fef19bed76b96/lib/app/client.js#L455-L457

Now it's not even going to be called? :-|

@Elevista
Copy link
Author

@ericmuyser
data function will be called only once in keep-alive page.
but asyncData is being called every time it active and makes nothing change
because it will be ignored here https://github.com/nuxt/nuxt.js/blob/f3d29e378087b6c6c686a03292c2bce409161110/packages/vue-app/template/utils.js#L24-L31
If there's something heavy ajax job in asnycData, it's really a problem.

Key of keep-alive page is route url, so if params.id is in url,
It will create new instance for each params.id. This is not going to be your problem.

@stale
Copy link

stale bot commented Aug 30, 2019

Thanks for your contribution to Nuxt.js!
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If you would like this issue to remain open:

  1. Verify that you can still reproduce the issue in the latest version of nuxt-edge
  2. Comment the steps to reproduce it

Issues that are labeled as pending will not be automatically marked as stale.

@stale stale bot added the stale label Aug 30, 2019
@stale stale bot closed this Sep 10, 2019
@Atinux Atinux reopened this Sep 17, 2019
@stale stale bot removed the stale label Sep 17, 2019
@541xxx
Copy link

541xxx commented Oct 21, 2019

It's been a long time, could this merge request be reviewed or merged?

@galvez galvez changed the title fix(vue-app): prevent calling asyncData when keep-alive page activated (#5945) fix(vue-app): prevent calling asyncData in keep-alive pages (#5945) Nov 24, 2019
@pi0 pi0 force-pushed the dev branch 2 times, most recently from 007c785 to a84f31d Compare January 21, 2020 12:42
@Elevista
Copy link
Author

Elevista commented Mar 6, 2020

This was wrong approach..

@Elevista Elevista closed this Mar 6, 2020
@danielroe danielroe added the 2.x label Jan 18, 2023
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.

calling asyncData multiple times when keep-alive page activated
7 participants