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

fix(core): stable route sorting #4331

Merged
merged 2 commits into from Nov 14, 2018
Merged

fix(core): stable route sorting #4331

merged 2 commits into from Nov 14, 2018

Conversation

pimlie
Copy link

@pimlie pimlie commented Nov 14, 2018

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

This PR fixes route sorting in Node v11 (fix #4188).

The problem was caused because the route sorting in Nuxt was inherently unstable due to default ordering to be dictated by the order in which elements where compared. This caused issues in v11 because quicksort compares arr[0] with arr[1] but timsort does arr[0] with arr[arr.length - 1].
E.g. if you look at the dynamic-routes test then with quicksort it compared parent to test but in timsort it compared test to parent. As the default behaviour of route sorting was to return 1, this meant that if there was no 'special' case a was always sorted after b regardless of its actual value (hence the inherently unstable). This PR changes this behaviour to alphabetically sort a and b (with localeCompare) if there is no 'special' case.

I have also moved route sorting outside the forEach loop, there is no need to sort the routes array on every intermediate step and it probably saves a little bit of time to only sort it once.

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.

@pi0 pi0 requested review from clarkdo and Atinux November 14, 2018 12:46
clarkdo
clarkdo previously approved these changes Nov 14, 2018
Copy link
Member

@clarkdo clarkdo left a comment

Choose a reason for hiding this comment

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

LGTM

packages/common/src/utils.js Outdated Show resolved Hide resolved
manniL
manniL previously approved these changes Nov 14, 2018
@pimlie pimlie dismissed stale reviews from manniL and clarkdo via 47691ba November 14, 2018 13:59
@codecov-io
Copy link

codecov-io commented Nov 14, 2018

Codecov Report

Merging #4331 into dev will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #4331      +/-   ##
==========================================
+ Coverage   91.05%   91.09%   +0.03%     
==========================================
  Files          56       56              
  Lines        1845     1852       +7     
  Branches      466      467       +1     
==========================================
+ Hits         1680     1687       +7     
  Misses        154      154              
  Partials       11       11
Impacted Files Coverage Δ
packages/common/src/utils.js 98.63% <100%> (+0.05%) ⬆️
packages/webpack/src/config/base.js 95.31% <0%> (-0.08%) ⬇️

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 3702dfe...47691ba. Read the comment docs.

@pimlie
Copy link
Author

pimlie commented Nov 14, 2018

Azure fails (again, intermittently) on the components test. The route sorting should be stable now between v10 and v11. Maybe we can add v11 testing to CI before merging this to make sure it does?

@clarkdo
Copy link
Member

clarkdo commented Nov 14, 2018

I will trigger azure rebuild.
I think we can merge it asap and I’ll add v11 into CI shortly.

@clarkdo clarkdo merged commit 846455e into nuxt:dev Nov 14, 2018
@lock
Copy link

lock bot commented Dec 14, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 14, 2018
@pimlie pimlie deleted the fix-route-sort branch January 16, 2019 19:54
@danielroe danielroe added the 2.x label Jan 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node v11 breaks route sorting
5 participants