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 unexpected state resetting #1408 #1914

Merged
merged 1 commit into from
Oct 31, 2017
Merged

fix unexpected state resetting #1408 #1914

merged 1 commit into from
Oct 31, 2017

Conversation

dotneet
Copy link

@dotneet dotneet commented Oct 20, 2017

related issue #1408

getMatchedComponentsInstances() in utils.js returns all components that is matched to new path.
I think this is the root cause of this issue.

getChangedComponentsInstances() in this PR returns components that needs to call data().

This repo can be used to reproduce a problem.
https://github.com/dotneet/nuxt-issue-1408

@Atinux Atinux self-requested a review October 23, 2017 08:45
@dotneet
Copy link
Author

dotneet commented Oct 25, 2017

Can I do something for your review?

@dotneet
Copy link
Author

dotneet commented Oct 26, 2017

I found a bug that is caused with a specific case in this PR, and I'm going to fix the bug.
but before that, I want to ask @Atinux if this PR has a possibility to be merged.

@Atinux
Copy link
Member

Atinux commented Oct 27, 2017

Hi @dotneet

I think it could be merged yes, it seems legit, I let you fix the bug before merging.

@dotneet
Copy link
Author

dotneet commented Oct 29, 2017

@Atinux Thank you for replying. I've fixed the issue and pushed it now.

@codecov-io
Copy link

codecov-io commented Oct 29, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##              dev   #1914      +/-   ##
=========================================
- Coverage   89.32%     89%   -0.32%     
=========================================
  Files          14      14              
  Lines         946     946              
=========================================
- Hits          845     842       -3     
- Misses        101     104       +3
Impacted Files Coverage Δ
lib/core/nuxt.js 93.18% <0%> (-4.55%) ⬇️
lib/core/renderer.js 77.17% <0%> (-0.42%) ⬇️

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 c006512...f186cad. Read the comment docs.

@Atinux Atinux merged commit e15c92f into nuxt:dev Oct 31, 2017
@Atinux
Copy link
Member

Atinux commented Oct 31, 2017

Thank you @dotneet

You're helping use a lot :)

@Atinux
Copy link
Member

Atinux commented Oct 31, 2017

Ok I see, the problem is when we use query parameters, it does not update automatically now.

@dotneet
Copy link
Author

dotneet commented Oct 31, 2017

Thank you for feedback.

Exactly. query parameters affects components but we can't detect what components need to be updated. so we need to update all components. this causes state resetting again.

I think that the behavior caused by fixPrepatch() is useful almost but it's not natural when it compared to vue-router's specification. So I propose removing data() evaluation on afterEach hook or making data() evaluation optional.

I think data() evaluation on afterEach hook causes some problems:

  1. unexpected state resetting. (Nested Dynamic Route change causes parent route to re-render #1408)
  2. make portability low. for example, normally, people believe created(), mounted() are called after data() is called every time.
  3. surprise people that are familiar with vue-router.

these problems would produce many bugs.

Do you have any thought on this?

NOTE:
vue-router does not call data() when parameters or query is changed.
https://router.vuejs.org/en/advanced/navigation-guards.html

@Atinux
Copy link
Member

Atinux commented Nov 2, 2017

I invited you on our Slack so we can talk about it by message :)

@lock
Copy link

lock bot commented Nov 1, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Nov 1, 2018
@danielroe danielroe added the 2.x label Jan 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants