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): support app/router.scrollBehavior.js and deprecate scrollBehavior #6055

Merged
merged 22 commits into from
Jul 24, 2019

Conversation

atinux
Copy link
Member

@atinux atinux commented Jul 10, 2019

This PR allows to overwrite application and modules templates with the app/ directory directly.

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

Not really a feature, but a refactor to move the router scrollBehavior to router.scrollBehavior.js.

Why?

In order to stop serializing function and start opening another way of changing the scrollBehavior with a file.
It also avoid restarting the whole Nuxt application since it's actually a config related to the Vue app.

Before:

// nuxt.config.js
export default {
  router: {
    scrollBehavior: function (to, from, savedPosition) {
       return { x: 0, y: 0 }
    }
  }
}

After:

// app/router.scrollBehavior.js
export default function(to, from, savedPosition) {
  return { x: 0, y: 0 }
}

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly. (PR: v2.9.0 docs#1472)
  • I have added tests to cover my changes (if not applicable, please state why)
  • All new and existing tests are passing.

@galvez
Copy link

galvez commented Jul 10, 2019

YES, thank you -- I was getting close to going down the acorn rabbit hole just to write us a working serializeFunction. It's definitely much better to have these imported as live JavaScript instead.

@atinux
Copy link
Member Author

atinux commented Jul 10, 2019

I plan to do the same logic to most of the configuration option that have to be serialised (head, transition, etc) 😄

@codecov-io
Copy link

codecov-io commented Jul 10, 2019

Codecov Report

Merging #6055 into dev will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #6055      +/-   ##
==========================================
+ Coverage   95.65%   95.66%   +0.01%     
==========================================
  Files          82       82              
  Lines        2695     2702       +7     
  Branches      693      697       +4     
==========================================
+ Hits         2578     2585       +7     
  Misses         99       99              
  Partials       18       18
Flag Coverage Δ
#e2e 100% <ø> (ø) ⬆️
#fixtures 50.18% <92.3%> (+0.09%) ⬆️
#unit 92.82% <100%> (+0.01%) ⬆️
Impacted Files Coverage Δ
packages/vue-app/src/index.js 0% <ø> (ø) ⬆️
packages/config/src/config/_common.js 100% <ø> (ø) ⬆️
packages/config/src/options.js 100% <100%> (ø) ⬆️
packages/builder/src/builder.js 99.61% <100%> (ø) ⬆️

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 39e41ac...d9567bd. Read the comment docs.

pi0
pi0 previously approved these changes Jul 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.

Neat 👍

packages/config/src/options.js Outdated Show resolved Hide resolved
@pi0 pi0 changed the title feat(vue-app): Refactor to move scrollBehavior feat(vue-app): support app/router.scrollBehavior.js and deprecate scrollBehavior Jul 11, 2019
@atinux atinux mentioned this pull request Jul 15, 2019
@atinux
Copy link
Member Author

atinux commented Jul 15, 2019

I need to add support for watching app/**/*.(js|vue) when creating or removing file there.

@atinux
Copy link
Member Author

atinux commented Jul 15, 2019

Watching app/ is done, I also added dir.app key in config

@atinux
Copy link
Member Author

atinux commented Jul 15, 2019

We can now overwrites custom templates given in build.templates with app/${dst}.

Example of a module adding press/common/pages/source.vue with this.addTemplate(), the user could overwrite it by adding app/press/common/pages/source.vue.

Nuxt will automatically watch the app/ dir and handle file added/updated/removed for it so the module author won't have to deal with watchers.

@atinux
Copy link
Member Author

atinux commented Jul 24, 2019

Good catch for typings Kevin, I think it's fine if they have autocomplete and then Nuxt warning 🤔

@pi0 pi0 merged commit f7cb3da into dev Jul 24, 2019
@pi0 pi0 deleted the feat/router branch July 24, 2019 11:36
@pi0
Copy link
Member

pi0 commented Jul 24, 2019

2.9.0 notes updated to mention

@manniL
Copy link
Member

manniL commented Jul 24, 2019

I really like the fact that we avoid serialization with that step, however (as briefly discussed with @atinux), this means we can't provide data from the nuxt.config.js inside such a external file in app (at the moment at least). For scrollBevahior, this might not have a huge impact, however for further split ups of things like transitions or head it could create a few problems if we don't handle it. ☺️

atinux added a commit that referenced this pull request Jul 30, 2019
@pi0 pi0 mentioned this pull request Aug 12, 2019
7 tasks
pi0 pushed a commit that referenced this pull request Aug 20, 2019
@danielroe danielroe added the 2.x label Jan 18, 2023
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.

8 participants