fix(vue-app): consider watchQuery option in routerViewKey

[hatashiro]

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

According to the Nuxt documentation, the default key provided to <nuxt-child> should be $route.fullPath. Please check the reference of nuxtChildKey in the following link:

• https://nuxtjs.org/api/components-nuxt#the-lt-nuxt-gt-component

However, the current code seems like using $route.path. Because of that, transition is not fired correctly when moving between the same page component, by changing query while keeping path the same.

I guess the documentation is describing the correct behavior (fullPath) and the current code should be changed.

It's my first contribution on Nuxt, so please feel free to guide me if I did something wrong :)

References

• [2.0]transition didn't work between same component change vuejs/vue-router#802
• Configurable 'key' attribute/property for transitions #1022

Commits

This p-r has 4 commits.

1. Before I work on the fix, I investigated test cases. I found the current scroll-to-top test is only checking scrollToTop: false, so I added a test case to check scrollToTop: true case.
2. Before I add a failing test case, I found that there is no helper method on browser util to go back. I added go() helper method to browser test util.
3. The last 2 commits are to add a failing test case and actually fix the issue.

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]

Hi @utatti and thank you for your PR.

I remember having something like this by default for a while, you can see some history here: #1022

You can see in this issue why I had to change the behaviour: #1255 (comment)

This is not as simple as it looks like when doing such changes :/

But I do like what you did on the tests, and I think we should be able to:

• Use a computed property to use $route.fullPath
• Test this behaviour
• Update the docs to explain what is the actual default and how to customize it

[hatashiro]

So if I understand correctly, we keep the current behaviour and update the doc? I also thought that using the key option should be mentioned in the doc too.

Also, I still think that triggerScroll should at least work with query changes specified in watchQuery. What so you think?

[hatashiro]

If using fullPath is too radical, I'd like to suggest using watchQuery like below:

let key = this.$route.path
if (Component && Component.options && Component.options.watchQuery) {
  const watchQuery = Component.options.watchQuery
  if (watchQuery === true) {
    key = this.$route.fullPath
  } else if (Array.isArray(watchQuery)) {
    key += `?${watchQuery.map(key => `${key}=${this.$route.query[key]}`).join('&')}`
  }
}
return key

The reason why I'd like to add a way to consider query in building key is that saving/restoring scroll position doesn't work currently when only query is changed. It's basically because query change is not considered as a vue-router transition (thus no beforeEnter hook), because they use the same key.

I understand reusing component is important, but I guess considering watchQuery is still an option?

Let me know your thought about the change above. I'll add follow-up fixes and test cases.

Cheers

[codecov-io]

Codecov Report

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

@@           Coverage Diff           @@
##              dev    #5516   +/-   ##
=======================================
  Coverage   95.67%   95.67%           
=======================================
  Files          81       81           
  Lines        2635     2635           
  Branches      671      671           
=======================================
  Hits         2521     2521           
  Misses         96       96           
  Partials       18       18

---

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 9346df7...cddf863. Read the comment docs.

[hatashiro]

I've updated the p-r and now it's to consider watchQuery on building routerViewKey, as I mentioned above. The implementation detail is a bit different though.

I've also added more test cases.

[atinux]

Hi @utatti

Thank you so much for investigating into this, I like your changes actually!

Could you also help to update the documentation to explain a bit more how it works, then I am ready to merge this PR :)

(https://github.com/nuxt/docs/blob/master/en/api/components-nuxt.md)

[hatashiro]

@atinux

Thanks! I've just uploaded a p-r on docs: nuxt/docs#1331

[manniL]

As @clarkdo correctly mentioned in nuxt/docs#1331, this is a breaking change.

[atinux]

Actually it's not really a breaking change, but mostly an improvement in the DX about transitions and scrollToTop from my POV

[pi0]

@clarkdo Can you please re-review this PR?

[clarkdo]

Not a breaking change, new option watchQuery as great enhancement.

All looks good, expect tiny change in my opinion.

[clarkdo]

If watchQuery is [], is router.resolve or fullPath supposed to be the expected value ?

fix(vue-app): consider watchQuery option in routerViewKey (#5516)

[atinux]

Good for me now

[galvez]

LGTM aside from formatting issues which I've commented on.

[hatashiro]

Thanks all!

[clarkdo]

Hi @utatti , as #5516 reported, this pr is a breaking change in some case.

How do you think change the fullPath option more compatible ?

Actually your requirement can be implemented by:

export default {
  key(route) {
    return route.fullPath
  }
}

[hatashiro]

@clarkdo Yes, this is a breaking change in that manner.

I understand we can use the key option, but I still think that it's natural to consider watchQuery on creating routerViewKey.

According to the watchQuery documentation:

If the defined strings change, all component methods (asyncData, fetch, validate, layout, ...) will be called.

However, only the scroll restoration and transition hooks are not working with watchQuery. If there is a better way to make them work, I'm good with it too.

I understand the final decision is up to maintainers, but I personally think the current behaviour is natural and the case like #5738 (where watchQuery is used but the component should be reused) should be handled as an exception.

[dhritzkiv]

@utatti Hmm. I would argue that re-using the component as much as possible should not be the exception, but is instead the Vue way.

As it stands, there is no way to achieve the old behaviour in 2.7.x, but under 2.6.x, you should have been able to achieve a Nuxt-child key override with key. If bar was the query string in watchQuery that you wanted to invalidate the component (thus forcing a complete re-creation), then this should work:

// this PR's behaviour in v2.6.3
export default {
  key(route) {
    return route.path + route.query.bar
  }
}

[hatashiro]

I understand that we can override the route key using key(route) in 2.6.x. The issue is not if we can achieve it or not, but how we achieve it.

I also agree that re-using components is essential, but the key discussion here is what watchQuery results in:

1. 2.6.x: asyncData, fetch, validate, layout, ..., most of data and methods are re-calculated and re-called, but no transition hook nor scroll restoration.
2. 2.7.x: Everything is re-created, works as usual page transition between different page components.

What I meant was 2 seemed natural to me than 1. I didn't mean that re-using component is exceptional.

I'm actually good to revert the change if scroll restoration works at least somehow with watchQuery. In 2.6.x, scroll restoration doesn't work between navigation in the same page component, and scroll is not set back to top on queried transition whatever we do (even scrollToTop doesn't do the job). Queried transition in the same component is actually a super common case: imagine building a paginated component with a page=n query.

All in all, I agree with you that reusing components is important and we should avoid breaking changes.

[hatashiro]

Today I'll investigate if there is a better way that we reuse page components and make scroll restoration work at the same time.