fix(vue-app): this.$nuxt.refresh doesn't refresh every fetch hooks #9530

ms-fadaei · 2021-07-07T20:32:05Z

With this fix, layout fetch, layout child components fetch (including the page), and page child component fetch will be called during the Nuxt refresh helper.

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

In the current version of the Nuxt when we call $nuxt.refresh(), the page fetch will be called if the current page has a fetch, otherwise, the child components fetch method will be called.

with this fix:

if the layout is enabled:
- if the layout itself has the fetch, it will be pushed to the call list
- if the layout child components (including the page) have the fetch, they will be pushed to the call list
if the layout is disabled:
- if the page itself has the fetch, it will be pushed to the call list
- if the page child components have the fetch, they will be pushed to the call list

Caution: What about nested routes? layout fetch will be inserted twice? can use set instead of array to prevent duplication?
fix: #9439

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.

codecov-commenter · 2021-07-07T20:34:13Z

Codecov Report

Merging #9530 (0a117cb) into dev (123206c) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##              dev    #9530   +/-   ##
=======================================
  Coverage   65.19%   65.19%           
=======================================
  Files          94       94           
  Lines        4155     4155           
  Branches     1172     1172           
=======================================
  Hits         2709     2709           
  Misses       1167     1167           
  Partials      279      279

Flag	Coverage Δ
unittests	`65.19% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out 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 123206c...0a117cb. Read the comment docs.

packages/vue-app/template/App.js

pi0 · 2021-07-07T20:44:00Z

packages/vue-app/template/App.js

-        } else {
-          // Get all component instance to call $fetch
+        // get layout component
+        const layout = this.$children.find((component) => component._name?.toLowerCase().includes("layouts"))


Refactor suggest: Maybe we can use let root = page and change it to parent layout instance if exists? (this way we merge to branches)

I think this also makes sense to only crawl everything once, also keeping getChildrenComponentInstancesUsingFetch simple. What was your opinion on that @ms-fadaei?

pi0 · 2021-07-07T20:49:37Z

What about nested routes? layout fetch will be inserted twice? can use set instead of array to prevent duplication?

Can you please ensure about this? If is a case, we might make use of WeakSet to dedup. (Note: IE support 🤷)

Also can you please check behavior when page does not having $fetch but layout does?

ms-fadaei · 2021-07-07T20:56:06Z

Can you please ensure about this? If is a case, we might make use of WeakSet to dedup. (Note: IE support 🤷)

Also can you please check behavior when page does not having $fetch but layout does?

I checked that right now. it not working properly with nested routes (then I should refactor the code)
I checked that too and it seems everything is right!

… into fix/layout-fetch

ms-fadaei · 2021-07-08T03:01:30Z

@pi0
what you thinking about getChildrenComponentInstancesUsingFetch
https://github.com/nuxt/nuxt.js/blob/HEAD/packages/vue-app/template/utils.js#L56-L68

If a child has the fetch, children of it will not be considered!

ms-fadaei · 2021-07-08T03:34:59Z

And I realized the sequence call of fetch (first parent then child) is not guaranteed! (in the current version this is just about nested routes because of limited fetch calls but in my version, this occur in every parent-child relations)

ms-fadaei · 2021-07-10T16:22:51Z

@pi0 @clarkdo I'm done with this PR.

We still have two edge cases that I mentioned above with refresh handler and they aren't new! they exist from the last versions

lihbr · 2021-07-27T08:43:39Z

Hey friends, what's up on this one?
What remains to be changed/discussed to lead to a merge? or is it just awaiting a review?

Cheers!

danielroe

I think the main thing we still need to handle is:

ensure that we don't duplicate fetches (for example if a component with fetch is included in both a page/layout) - see fix(vue-app): this.$nuxt.refresh doesn't refresh every fetch hooks #9530 (comment) for one approach

lihbr · 2021-11-29T10:50:55Z

I'll try to have a look at the last bit @danielroe mentioned this week 💪 Keeping you up-to-date!

lihbr · 2021-12-02T15:51:24Z

OK, I'm having a bit of trouble understanding the "duplicate fetch" maybe issue mentioned above.

Looking at the changes, 3 things have been achieved:

getChildrenComponentInstancesUsingFetch() has been modified to handle an exclude array, stopping the crawling if a node from exclude is encountered during the crawling process;
On the pages.map(), the callback now loop only over the children component of the current page, theoretically deduplicating redundant calls (? not 100% confident on that one, but that's what I understand);
After the pages.map(), the refresh() method now looks for a layout, if one it adds its own fetch and crawls its children component, excluding pages and their offspring.

Taking all of those changes individually they seem pretty straightforward to me. Basically, the refresh() method now achieve that kind of resolution for fetch hooks (I had budget for an animation today):

I don't really see how we can end up with duplicate calls here, or at least how we can end up with duplicate calls we didn't have already before(?)

Now, maybe where I'm wrong is on my understanding of the getMatchedComponentsInstances() function. To me, that function gets each page component actually displayed on the current route, so from my gif above ~/pages/blog.vue & ~/pages/blog/index.vue? I'm not sure about that function behavior as it seems to access properties of Vue Router current route which I'm not aware of/are Nuxt specific(?): https://router.vuejs.org/api/#route-object-properties

If I'm wrong here and there's definitely something leading to duplicated behaviors, I'm not sure however a set would solve the issue? Or maybe it would? I'd be afraid only some of the deduplicated hooks get called and that the others remain stale (a.K.a. no new data?)

Let me know!

pi0 · 2022-06-23T23:11:26Z

@ms-fadaei @lihbr Can you please check the latest nuxt-edge? I believe #10489 should address the issues by refetching on all children. If not happy to reopen and improve 👍🏼

lihbr · 2022-06-24T08:20:41Z

Hey there,

Unfortunately, this doesn't appear to fix the issue as the component-crawling function still starts from the page component instead of the layout.

Maybe I'm doing something wrong, here's my repro on edge: nuxt_nuxt.js_9530_1.zip

pi0 · 2022-06-24T08:28:14Z

Thanks for checking @lihbr. @ms-fadaei Would you please rebase your branch to dev?

ms-fadaei · 2022-06-24T09:03:13Z

Thanks for checking @lihbr. @ms-fadaei Would you please rebase your branch to dev?

Hi Pooya,
Done!

lihbr · 2022-06-24T09:16:14Z

packages/vue-app/template/App.js

@@ -214,6 +221,24 @@ export default {

        return Promise.all(p)
      })
+
+      <% if (features.fetch) { %>


Should this be considered again here? From #10489

https://github.com/nuxt/nuxt.js/pull/10489/files#diff-45fb7db1f2da36defea7d4cf1f2a5f4db0a8f9f320b889808f204e51e4df8259R201-R202

fix(vue-app): this.$nuxt.refresh doesn't refresh every fetch hooks

1ef8d4f

clarkdo reviewed Jul 7, 2021

View reviewed changes

packages/vue-app/template/App.js Outdated Show resolved Hide resolved

pi0 reviewed Jul 7, 2021

View reviewed changes

Merge branch 'dev' into fix/layout-fetch

2b5fa57

ms-fadaei added 2 commits July 8, 2021 01:51

fix: move layout fetch to outside the page section

925701b

Merge branch 'fix/layout-fetch' of https://github.com/ms-fadaei/nuxt.js…

3f17a7e

… into fix/layout-fetch

ms-fadaei added 2 commits July 8, 2021 08:25

fix: exclude page component and its children in layout fetch

d280341

test: increment MODERN_JS_RESOURCES_BROTLI_KB_SIZE

1ef4960

danielroe and others added 2 commits July 27, 2021 09:47

Merge branch 'dev' into fix/layout-fetch

48b4751

Merge branch 'dev' into fix/layout-fetch

03b674b

pi0 mentioned this pull request Aug 11, 2021

v2.16.0 #9660

Closed

Atinux requested a review from danielroe September 21, 2021 09:10

danielroe reviewed Sep 21, 2021

View reviewed changes

Merge branch 'dev' into fix/layout-fetch

4134c72

Merge branch 'dev' into fix/layout-fetch

b335161

lihbr mentioned this pull request Feb 28, 2022

Vue: Prismic preview doesn't work with partials (nav, header, footer, etc.) prismicio/slice-machine#33

Closed

lihbr mentioned this pull request Mar 16, 2022

Layouts Preview does not work when building in full static mode nuxt-modules/prismic#154

Closed

pi0 closed this Jun 23, 2022

pi0 reopened this Jun 24, 2022

saleh added 2 commits June 24, 2022 13:30

Merge branch 'dev' of https://github.com/nuxt/nuxt.js into nuxt-dev

cffcb5a

Merge branch 'nuxt-dev' into fix/layout-fetch

0a117cb

lihbr reviewed Jun 24, 2022

View reviewed changes

danielroe added the 2.x label Jan 18, 2023

danielroe added the bug label Feb 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(vue-app): this.$nuxt.refresh doesn't refresh every fetch hooks #9530

fix(vue-app): this.$nuxt.refresh doesn't refresh every fetch hooks #9530

ms-fadaei commented Jul 7, 2021

codecov-commenter commented Jul 7, 2021 •

edited

pi0 Jul 7, 2021 •

edited

lihbr Jul 27, 2021

pi0 commented Jul 7, 2021 •

edited

ms-fadaei commented Jul 7, 2021

ms-fadaei commented Jul 8, 2021

ms-fadaei commented Jul 8, 2021

ms-fadaei commented Jul 10, 2021

lihbr commented Jul 27, 2021

danielroe left a comment

lihbr commented Nov 29, 2021

lihbr commented Dec 2, 2021

pi0 commented Jun 23, 2022

lihbr commented Jun 24, 2022

pi0 commented Jun 24, 2022

ms-fadaei commented Jun 24, 2022 •

edited

lihbr Jun 24, 2022

fix(vue-app): this.$nuxt.refresh doesn't refresh every fetch hooks #9530

Are you sure you want to change the base?

fix(vue-app): this.$nuxt.refresh doesn't refresh every fetch hooks #9530

Conversation

ms-fadaei commented Jul 7, 2021

Types of changes

Description

Checklist:

codecov-commenter commented Jul 7, 2021 • edited

Codecov Report

pi0 Jul 7, 2021 • edited

Choose a reason for hiding this comment

lihbr Jul 27, 2021

Choose a reason for hiding this comment

pi0 commented Jul 7, 2021 • edited

ms-fadaei commented Jul 7, 2021

ms-fadaei commented Jul 8, 2021

ms-fadaei commented Jul 8, 2021

ms-fadaei commented Jul 10, 2021

lihbr commented Jul 27, 2021

danielroe left a comment

Choose a reason for hiding this comment

lihbr commented Nov 29, 2021

lihbr commented Dec 2, 2021

pi0 commented Jun 23, 2022

lihbr commented Jun 24, 2022

pi0 commented Jun 24, 2022

ms-fadaei commented Jun 24, 2022 • edited

lihbr Jun 24, 2022

Choose a reason for hiding this comment

codecov-commenter commented Jul 7, 2021 •

edited

pi0 Jul 7, 2021 •

edited

pi0 commented Jul 7, 2021 •

edited

ms-fadaei commented Jun 24, 2022 •

edited