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: keep-alive component data should not be updated #5188

Merged
merged 1 commit into from Mar 8, 2019

Conversation

clarkdo
Copy link
Member

@clarkdo clarkdo commented Mar 8, 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

Fix #5166

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.

@clarkdo clarkdo requested review from Atinux and pi0 March 8, 2019 12:19
@codecov-io
Copy link

Codecov Report

Merging #5188 into dev will decrease coverage by 1.47%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #5188      +/-   ##
==========================================
- Coverage   96.52%   95.05%   -1.48%     
==========================================
  Files          72       72              
  Lines        2447     2447              
  Branches      620      620              
==========================================
- Hits         2362     2326      -36     
- Misses         70      100      +30     
- Partials       15       21       +6
Impacted Files Coverage Δ
packages/webpack/src/plugins/vue/cors.js 0% <0%> (-90%) ⬇️
packages/webpack/src/config/client.js 83.33% <0%> (-12.97%) ⬇️
packages/webpack/src/plugins/vue/server.js 79.31% <0%> (-6.9%) ⬇️
packages/webpack/src/utils/postcss.js 85.71% <0%> (-6.13%) ⬇️
packages/webpack/src/utils/style-loader.js 88.23% <0%> (-5.89%) ⬇️
packages/webpack/src/plugins/vue/client.js 88.57% <0%> (-5.72%) ⬇️
packages/webpack/src/utils/perf-loader.js 94.73% <0%> (-5.27%) ⬇️
packages/webpack/src/config/base.js 90.9% <0%> (-4.55%) ⬇️
packages/webpack/src/builder.js 91.48% <0%> (-3.2%) ⬇️
packages/utils/src/route.js 97.58% <0%> (-2.42%) ⬇️
... and 1 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 2cbddeb...85ec562. Read the comment docs.

@@ -491,6 +491,7 @@ function fixPrepatch(to, ___) {
if (
instance.constructor._dataRefresh &&
Components[i] === instance.constructor &&
instance.$vnode.data.keepAlive !== true &&
Copy link
Member

Choose a reason for hiding this comment

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

Just to be sure, is the default value of keepAlive, false by default? If it is true, isn't this a minor breaking change?

Copy link
Member Author

Choose a reason for hiding this comment

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

The default value is undefined, it's just a flag in Vue for keepAlive cached component.

I don't think it's a breaking change, it should be a bug because component under keep-alive should be cached and never reload data as Vue is doing.

Copy link

Choose a reason for hiding this comment

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

Just wondering, shouldn't it be data.props.keepAlive instead of data.keepAlive? Because in bunch of files (packages/vue-app/template/components/nuxt.js, etc), keepAlive property is being wrapped into props object.

I ran into an issue when component is still trying to update its data() even though it has keep-alived enabled.
Changing this line to data.props.keepAlive solved problem for me.

PS: Nuxt v2.15.2 (seems like nothing have been changed with keep-alived behaviour ever since this commit)

Copy link
Member Author

Choose a reason for hiding this comment

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

keepAlive here is checking vue keep-alive component which will add keepAlive to vnode.data https://github.com/vuejs/vue/blob/b51430f598b354ed60851bb62885539bd25de3d8/src/core/components/keep-alive.js#L120

Copy link

Choose a reason for hiding this comment

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

Thanks for your reply.

Sorry, but what should be the keep-alive component in here? Because debugger says that instance is a page component. Also, instance.$vnode.data doesn't have keepAlive property, but it does have props.keepAlive.

Screenshot:

20210301_015410

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the component in your screenshot is NuxtChild which has keep-alive prop for enabling keepAlive on it.

https://nuxtjs.org/docs/2.x/features/nuxt-components#keep-alive

Copy link

Choose a reason for hiding this comment

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

You right, it is NuxtChild component.
I thought it should work the same way for any components, but seems like it is not the case.
I'll try to investigate it a little bit further and maybe come back with new issue and/or PR.
Thanks for your comments 👍

@clarkdo clarkdo merged commit 1ea8661 into nuxt:dev Mar 8, 2019
@clarkdo
Copy link
Member Author

clarkdo commented Mar 8, 2019

Thanks, bro.

@clarkdo clarkdo deleted the ka-data branch March 8, 2019 16:43
@pi0 pi0 mentioned this pull request Mar 14, 2019
@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.

keep-alive not working, resets data after change tab
4 participants