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

feat(vue-app): add prefetch prop to <nuxt-link> #6292

Merged
merged 10 commits into from Sep 18, 2019
Merged

feat(vue-app): add prefetch prop to <nuxt-link> #6292

merged 10 commits into from Sep 18, 2019

Conversation

lmichelin
Copy link

@lmichelin lmichelin commented Aug 24, 2019

Type 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

Fixes #6135

<nuxt-link> currently allows disabling prefetching with a specific link using a no-prefetch prop, but on the other hand, there is no way to prefetch a specific link when links prefetching is disabled globally via nuxt.config.js with prefetchLinks: false.

This PR adds a prefetch prop to <nuxt-link> which can be used when prefetchLinks is set to false.

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly. (PR: docs(en): Add prefetch prop to <nuxt-link> docs#1550)
  • I have added tests to cover my changes (if not applicable, please state why)
  • All new and existing tests are passing.

@lmichelin lmichelin changed the title Add prefetch prop to nuxt link feat: Add prefetch prop to <nuxt-link> Aug 24, 2019
@codecov-io
Copy link

codecov-io commented Aug 24, 2019

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##             dev   #6292   +/-   ##
=====================================
  Coverage   95.6%   95.6%           
=====================================
  Files         79      79           
  Lines       2684    2684           
  Branches     692     692           
=====================================
  Hits        2566    2566           
  Misses       101     101           
  Partials      17      17
Flag Coverage Δ
#e2e 100% <ø> (ø) ⬆️
#fixtures 50.7% <ø> (ø) ⬆️
#unit 92.28% <ø> (ø) ⬆️

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 ac5066c...b65b4ca. Read the comment docs.

Copy link

@pimlie pimlie left a comment

Choose a reason for hiding this comment

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

My apologies to have to do this, but I have just added a compileTemplate test helper method I would like you to ask to use to add unit tests (using vue-test-utils) instead of adding new fixtures. Adding new fixtures is not preferred as building each fixtures takes at least 10 seconds to the total test time.

You can have a look at nuxt-loading.test.js for an example of an existing component unit test.

For this pr you could use the compileTemplate function as follows:

// nuxt-link.client.prefetch.test.js
const compiledTemplatePath = await compileTemplate('components/nuxt-link.client.vue', 'nuxt-link.client.prefetch.vue', { router: { prefetch: true } })

// nuxt-link.client.noprefetch.test.js
const compiledTemplatePath = await compileTemplate('components/nuxt-link.client.vue', 'nuxt-link.client.noprefetch.vue', { router: { prefetch: false } })

For quick reference, here is the link to the vtu docs

packages/vue-app/template/components/nuxt-link.client.js Outdated Show resolved Hide resolved
packages/vue-app/template/client.js Outdated Show resolved Hide resolved
@Atinux
Copy link
Member

Atinux commented Aug 26, 2019

What about prefetchLinks: “on-demand”?

@lmichelin
Copy link
Author

What about prefetchLinks: “on-demand”?

Maybe the best option is to wait for the PR #6287 to be merged?

pimlie
pimlie previously approved these changes Sep 10, 2019
Copy link

@pimlie pimlie left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Thanks for the recurring work you put into this pr!

@lmichelin
Copy link
Author

lmichelin commented Sep 11, 2019

My apologies to have to do this, but I have just added a compileTemplate test helper method I would like you to ask to use to add unit tests (using vue-test-utils) instead of adding new fixtures. Adding new fixtures is not preferred as building each fixtures takes at least 10 seconds to the total test time.

You can have a look at nuxt-loading.test.js for an example of an existing component unit test.

For this pr you could use the compileTemplate function as follows:

// nuxt-link.client.prefetch.test.js
const compiledTemplatePath = await compileTemplate('components/nuxt-link.client.vue', 'nuxt-link.client.prefetch.vue', { router: { prefetch: true } })

// nuxt-link.client.noprefetch.test.js
const compiledTemplatePath = await compileTemplate('components/nuxt-link.client.vue', 'nuxt-link.client.noprefetch.vue', { router: { prefetch: false } })

For quick reference, here is the link to the vtu docs

I tried to do some tests but I faced some issues mounting the <nuxt-link> component with vue-test-utils:

image

I think it's because nuxt-link extends routerLink but I didn't find a way to stub this component. 😕

So there is no test in this PR at the moment... 😕

@pimlie
Copy link

pimlie commented Sep 11, 2019

Its probably because extends: Vue.component('RouterLink') fails. Did you add a stub or fake router-link component? See: https://vue-test-utils.vuejs.org/api/components/#routerlinkstub

@lmichelin
Copy link
Author

I tried but it has no effect 😕

@pimlie
Copy link

pimlie commented Sep 11, 2019

What about something like this?

import { createLocalVue } from '@vue/test-utils'

const localVue = createLocalVue()
// register stub component so nuxt-link extends its render fn
localVue.component('RouterLink', {
  render: h => h('a')
})
// load nuxt-link

pi0
pi0 previously approved these changes Sep 11, 2019
Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

LGTM

@pi0 pi0 changed the title feat: Add prefetch prop to <nuxt-link> feat: ad prefetch prop to <nuxt-link> Sep 11, 2019
@lmichelin lmichelin dismissed stale reviews from pi0 and pimlie via 561770a September 11, 2019 08:00
@lmichelin lmichelin changed the title feat: ad prefetch prop to <nuxt-link> feat: add prefetch prop to <nuxt-link> Sep 11, 2019
@lmichelin
Copy link
Author

What about something like this?

import { createLocalVue } from '@vue/test-utils'

const localVue = createLocalVue()
// register stub component so nuxt-link extends its render fn
localVue.component('RouterLink', {
  render: h => h('a')
})
// load nuxt-link

Still face the issue 😕, I made a WIP commit to show you the code :) (I also needed to fix a bug in the compileTemplate helper)

@pimlie
Copy link

pimlie commented Sep 11, 2019

Hmm, could you try to explicitly set the router-link component as a stub in the mounting options as described here?

@lmichelin
Copy link
Author

Still facing the warning 😕
I tried : stubs: ['router-link'], stubs: ['RouterLink'], stubs: { 'router-link': true }

@pimlie
Copy link

pimlie commented Sep 12, 2019

@lmichelin Hope you dont mind, but as I didnt knew the correct answer myself I just tried fixing this on your branch. The tests should work now.

Feel free to improve them further if you wish!

@lmichelin
Copy link
Author

Nice! Thanks a lot for your help 😃

@lmichelin
Copy link
Author

lmichelin commented Sep 12, 2019

And btw do you agree with the fix in compileTemplate: 374363a ? I needed to do this to avoid file overwriting during tests 😉

@pimlie
Copy link

pimlie commented Sep 12, 2019

Yes, a bit sily to have destination as fn arg but not use it. Thanks for the fix ;)

I have added one more test condition btw, thought it was a bit disconcerting that VTU only logged an error but didnt seem to return/throw any error about the failed mount. So have added some spies on the console to make sure no errors or warnings are logged.

@lmichelin Let us know btw if you think this PR is now ready or if you wish to make some other changes, thanks :)

@lmichelin
Copy link
Author

Do you want me to clean the commit history to squash some commits and remove WIP in messages?

pimlie
pimlie previously approved these changes Sep 12, 2019
@pimlie
Copy link

pimlie commented Sep 12, 2019

@lmichelin Thanks but dont think that is necessary. The pr will be squashed when merged already so those commits wont show up on the dev branch anyway.

@Atinux
Copy link
Member

Atinux commented Sep 13, 2019

Great work here @lmichelin 👏

One thing came to my mind when I read prefetch and no-prefetch props, what about deprecating no-prefetch in favour of :prefetch="false"?

@lmichelin
Copy link
Author

I asked the question here #6292 (comment) but @pi0 preferred not to deprecate it.

@lmichelin
Copy link
Author

@lmichelin Let us know btw if you think this PR is now ready or if you wish to make some other changes, thanks :)

It's ok for me!

@pi0 pi0 changed the title feat: add prefetch prop to <nuxt-link> feat(vue-app): add prefetch prop to <nuxt-link> Sep 18, 2019
@pi0 pi0 changed the title feat(vue-app): add prefetch prop to <nuxt-link> feat(vue-app): add prefetch prop to <nuxt-link> Sep 18, 2019
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.

Allow prefetch prop for nuxt-link to enable prefetching for specific links
7 participants