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(vue-app): pass router mode to getLocation #6658

Merged
merged 3 commits into from Nov 24, 2019
Merged

fix(vue-app): pass router mode to getLocation #6658

merged 3 commits into from Nov 24, 2019

Conversation

rchl
Copy link

@rchl rchl commented Nov 6, 2019

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

On the client, Nuxt creates "synthetic" route initially, before VueRouter
has initialized so that plugins and middlewares have something to work
with at that early stage already. The problem was that initial route
was created without taking router's mode into consideration so it
created route with normal path rather than hash-based path. That confused
nuxt-i18n's logic that tries to redirect user to correct locale on
initial navigation.

As for tests:

  • created new spa-hash fixture config so that we can run tests in hash mode
  • new fixture is using files from spa fixture to avoid duplication
  • change spa.test.js testsuite to run same tests on both fixtures
  • disable /тест雨 (test non ascii route) test in hash mode as it is failing
    for unrelated reason (I haven't investigated deeper).
  • add Initial route has correct fullPath test to test the bug fixed here

Resolves nuxt-modules/i18n#523

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.

On the client, Nuxt creates "synthetic" route initially, before VueRouter
has initialized so that plugins and middlewares have something to work
with at that early stage already. The problem was that initial route
was created without taking router's `mode` into consideration so it
created route with normal path rather than hash-based path. That confused
nuxt-i18n's logic that tries to redirect user to correct locale on
initial navigation.

As for tests:
 - created new `spa-hash` fixture config so that we can run tests in hash mode
 - new fixture is using files from `spa` fixture to avoid duplication
 - change `spa.test.js` testsuite to run same tests on both fixtures
 - disable `/тест雨 (test non ascii route)` test in hash mode as it is failing
   for unrelated reason (I haven't investigated deeper).
 - add `Initial route has correct fullPath` test to test the bug fixed here

Resolves nuxt-modules/i18n#523
@rchl
Copy link
Author

rchl commented Nov 6, 2019

Much more pleasant to review after enabling Hide whitespace changes option in github's diff view.

@codecov-io
Copy link

codecov-io commented Nov 6, 2019

Codecov Report

Merging #6658 into dev will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #6658      +/-   ##
==========================================
- Coverage   95.83%   95.79%   -0.04%     
==========================================
  Files          78       78              
  Lines        2713     2713              
  Branches      702      702              
==========================================
- Hits         2600     2599       -1     
- Misses         98       99       +1     
  Partials       15       15
Flag Coverage Δ
#e2e 100% <ø> (ø) ⬆️
#fixtures 64.65% <ø> (ø) ⬆️
#unit 90.96% <ø> (-1.37%) ⬇️
Impacted Files Coverage Δ
packages/vue-renderer/src/renderer.js 93.49% <0%> (-0.82%) ⬇️

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 587d1e9...e6843ae. Read the comment docs.

@rchl rchl changed the title fix(vue-app): initial "synthetic" route invalid when router in hash mode fix(vue-app): invalid initial "synthetic" route when router in hash mode Nov 6, 2019
@clarkdo clarkdo requested a review from Atinux November 9, 2019 22:45
@clarkdo clarkdo requested a review from a team November 9, 2019 22:49
return { window, head, html }
}
// Runs tests in specified router mode (either "hash" or "history").
function spaTests ({ isHashMode }) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we extract the hash mode specific test cases into a separted file like spa.hash.test.js for more readable and easier to maintain in future ?

Copy link
Author

Choose a reason for hiding this comment

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

There are no "hash specific" testcases per se. All are valid testcases for both modes (apart from one test which fails in hash mode for some unknown reason). So it wouldn't make sense to create two identical sets of tests and two identical sets of files in a new fixture.

@pi0 pi0 changed the title fix(vue-app): invalid initial "synthetic" route when router in hash mode fix(vue-app): pass router mode to getLocation Nov 24, 2019
@pi0 pi0 merged commit e9945b0 into nuxt:dev Nov 24, 2019
@rchl rchl deleted the fix/hash-mode-initial-route branch November 24, 2019 14:52
@pi0 pi0 mentioned this pull request Nov 26, 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.

Fails to redirect to locale on initial navigation with router in hash mode
5 participants