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 double layout execution (#5703) #7442

Merged
merged 6 commits into from
Jun 9, 2020

Conversation

galvez
Copy link

@galvez galvez commented May 31, 2020

In vue-app/client.js, we use showNextPage() to set layout, which is ran in an afterEach hook. As reported in #5703, this causes a double execution when switching layouts. Perhaps having this occur in afterEach is not fast enough -- switching to beforeResolve seems to solve it.

Update: using afterEach works, but the actual call must happen before nextTick.

@galvez galvez requested review from pimlie, Atinux and pi0 May 31, 2020 15:07
@codecov-commenter
Copy link

codecov-commenter commented May 31, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #7442   +/-   ##
=======================================
  Coverage   70.32%   70.32%           
=======================================
  Files          88       88           
  Lines        3704     3704           
  Branches     1010     1010           
=======================================
  Hits         2605     2605           
  Misses        892      892           
  Partials      207      207           
Flag Coverage Δ
#unittests 70.32% <ø> (ø)

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 b45f284...c6edad6. Read the comment docs.

@galvez
Copy link
Author

galvez commented May 31, 2020

Also reported in #7441.

@pimlie
Copy link

pimlie commented May 31, 2020

Does this change influence page transitions? Did you check manually? Dont think we have proper tests to make sure they perform well visually?

But Im worried that this might be a breaking change (albeit it probably a low risk one). If a user already adds their own beforeResolve hook and cancels navigation (or redirects) within that, then the new/wrong layout will already be set.

@galvez
Copy link
Author

galvez commented May 31, 2020

I did test manually, seems ok.

Re: beforeResolve(), I had an idea to workaround this, will try it next.

@galvez
Copy link
Author

galvez commented May 31, 2020

@pimlie turns out we can use afterEach for this, we just can't await for nextTick.

@simplenotezy
Copy link

🎉🎉

@pi0 pi0 merged commit 0acfc78 into dev Jun 9, 2020
@pi0 pi0 deleted the fix-layout-called-twice branch June 9, 2020 16:39
@pi0 pi0 mentioned this pull request Jun 10, 2020
@simplenotezy
Copy link

Great!! Good job.

@Atinux Can I pull this fix in through nuxt-edge or how can I get this fix in my application?

@pi0
Copy link
Member

pi0 commented Jun 10, 2020

@simplenotezy nuxt-edge releases are nightly so you should be able to test if it is fixing issue in your application.

@simplenotezy
Copy link

I see, thanks @pi0

@simplenotezy
Copy link

I can confirm it's working on my end. Good job guys

@ademyalcin27
Copy link

Hi,
I was using Nuxt@2.9.2 and which is mounted triggered 2 times in mobile. after that I upgraded my nuxt version from 2.9.2 to 2.13.3 I still got same error, Also It's same layout calling 2 times in mounted, 'VirtualPageViewAll', and 'mobileDefault' calling in mounted

Ekran Resmi 2020-07-07 13 42 15

@hjujah
Copy link

hjujah commented Jul 27, 2020

For me its not page but all page components are mounting twice. Tried Nuxt 2.13 and 2.14 and edge (2.15) but having the same issue everywhere. Please is there any fix for this?

image

https://streamable.com/n5m3y6 ---> Check the fix on this screen recording, but it would be great if someone could explain this!? After removing that commented line the components are not mounting twice anymore 😅

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.

None yet

9 participants