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(utils): export sort routes #6277

Merged
merged 2 commits into from Sep 1, 2019
Merged

Conversation

pimlie
Copy link

@pimlie pimlie commented Aug 22, 2019

(Helps to) resolve:

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

Well, not really a feature but also not a bug fix.

If a user calls extendRoutes and adds new routes, he/she is also responsible for maintaining proper order of the routes. This is quite a cumbersome task, by exposing the sortRoutes method a user can import it, add route to his/her liking and then easily sort them properly again.

(This is useful until vue-router fully supports auto path ranking)

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.

@pimlie pimlie requested a review from a team August 22, 2019 08:16
@codecov-io
Copy link

codecov-io commented Aug 22, 2019

Codecov Report

Merging #6277 into dev will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##             dev   #6277   +/-   ##
=====================================
  Coverage   95.7%   95.7%           
=====================================
  Files         79      79           
  Lines       2652    2652           
  Branches     683     683           
=====================================
  Hits        2538    2538           
  Misses        98      98           
  Partials      16      16
Flag Coverage Δ
#e2e 100% <ø> (ø) ⬆️
#fixtures 50.56% <100%> (ø) ⬆️
#unit 92.3% <100%> (ø) ⬆️
Impacted Files Coverage Δ
packages/utils/src/route.js 100% <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 b286024...1aeb89b. Read the comment docs.

@manniL
Copy link
Member

manniL commented Aug 22, 2019

Would it make sense to provide it as an argument for extendRoutes?

@pimlie
Copy link
Author

pimlie commented Aug 22, 2019

Yes, so people dont have to import from nuxt utils themselves
No, because of the expected support of vue-router for auto path ranking it might be better to not temporarily change Nuxt's API

@pimlie pimlie added this to Ready to Review in Nuxt v2.10 Aug 22, 2019
@manniL manniL requested a review from a team August 26, 2019 21:58
@Atinux
Copy link
Member

Atinux commented Aug 27, 2019

Nice PR @pimlie

Would it be even better if we call sortRoutes just after extendRoutes so they won't have to deal with sorting and just push the new route to the array?

@pimlie
Copy link
Author

pimlie commented Aug 27, 2019

It would be better probably, but are we 100% sure that sortRoutes doesnt contain any bugs at all and will sort all possible routes always perfectly? Unfortunately I think it doesnt, its too much meant to work with nuxt-routes which are often a bit simpler then the full range or possible routes with vue-router (eg regular expressions). Thats probably also why a lot of people will use extendRoutes anyway.

As vue-router seems to be already adding functionality for this, I think its best if we just provide a temporary fix until auto path ranking is merged in vue-router.

@Atinux
Copy link
Member

Atinux commented Aug 27, 2019

Did not think of this, you are right @pimlie

Ready to merge 👍

@pi0 pi0 merged commit 0daaf87 into nuxt:dev Sep 1, 2019
@pimlie pimlie deleted the feat-export-sortroutes branch September 4, 2019 07:17
@pimlie pimlie moved this from Ready to Review to Done in Nuxt v2.10 Sep 4, 2019
@bravokiloecho
Copy link

Is there any documentation of how to use this anywhere?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Nuxt v2.10
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

9 participants