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

hotfix: fix issues with url handling #8494

Merged
merged 7 commits into from Dec 16, 2020
Merged

hotfix: fix issues with url handling #8494

merged 7 commits into from Dec 16, 2020

Conversation

pi0
Copy link
Member

@pi0 pi0 commented Dec 15, 2020

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

🚂 History: vuejs/vue-router#3350 > #8325 > #8421 > #8430 > #8462

Resolves: #8497, #8493, #8458, #8457

ℹ️ Update ufo to 0.5.x:

  • Use custom parser instead of depending on global URL parser
  • Remove url-polyfill
  • Encode with same utils of vue-router-next
  • browser filed removed as UMD cannot be used for named exports and we have same implementation for both envs
  • Add missing @nuxt/ufo dependency to @nuxt/vue-renderer to avoid inlining

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.

@codecov-io
Copy link

codecov-io commented Dec 15, 2020

Codecov Report

Merging #8494 (a21abb2) into 2.x (d85f4b9) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              2.x    #8494      +/-   ##
==========================================
- Coverage   68.23%   68.22%   -0.02%     
==========================================
  Files          91       91              
  Lines        3904     3902       -2     
  Branches     1066     1065       -1     
==========================================
- Hits         2664     2662       -2     
  Misses       1005     1005              
  Partials      235      235              
Flag Coverage Δ
unittests 68.22% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/webpack/src/config/client.js 69.44% <ø> (-0.83%) ⬇️

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 d85f4b9...a21abb2. Read the comment docs.

@pi0 pi0 changed the base branch from dev to 2.x December 15, 2020 23:23
@pi0 pi0 marked this pull request as ready for review December 16, 2020 10:16
@pi0 pi0 requested review from a team and danielroe and removed request for a team December 16, 2020 10:17
@danielroe
Copy link
Member

A few issues I've found so far:

  1. IE behaviour in encoding fixture to non-URL-encoded direct dynamic link doesn't match on server side but this may be IE issue rather than ufo:

    /%C3%B6/dynamic/سلام چطوری?q=cofee,food,دسر
    
    Screenshot

    Screenshot from 2020-12-16 10-40-11

  2. Edge 18 has a hydration issue on /тест direct and doesn't recognise matches for the non-URL-encoded direct dynamic link

  3. Safari 5.1 has an issue with most URLs on client-side (indicating differing behaviour between node/client code) - but this is likely too old for us to support

However, evergreen browsers all appear to work with consistent behaviour, and I think the issues highlighted here would probably have been present without @nuxt/ufo. With a browser testing suite, we may also be able to identify and address within the ufo project.

@pi0 pi0 marked this pull request as draft December 16, 2020 11:17
@pi0
Copy link
Member Author

pi0 commented Dec 16, 2020

@danielroe Thanks for investigation.

Quickly checking IE issue, it seems that is original URL sent by IE is wrong (i think it was an issue before vue-router update as well)

  • ssrContext.url /dynamic/سلام چطوری?q=cofee,food,دسر (which is incorrect)
  • normalized: /dynamic/%D8%B3%D9%84%D8%A7%D9%85%20%DA%86%D8%B7%D9%88%D8%B1%DB%8C?q=cofee,food,%C3%98%C2%AF%C3%98%C2%B3%C3%98%C2%B1 (which is correct but input was incorrect :D)

For encoding fixture, (direct) items are directly using <a> tag (without ufo normalization) while we do normalize for NuxtLink items so clicking on them or opening in new tab always works since ufo normalized links. So for all browsers support users have to use RouterLink/Nuxtlink or encode themselves for <a> tags

@pi0 pi0 marked this pull request as ready for review December 16, 2020 13:11
@pi0 pi0 merged commit a25bdd6 into 2.x Dec 16, 2020
@pi0 pi0 deleted the fix/url branch December 16, 2020 13:12
@gkatsanos
Copy link

gkatsanos commented Dec 16, 2020

hey guys, thank you for fixing this! What is the release process now?

@pi0
Copy link
Member Author

pi0 commented Dec 16, 2020

@gkatsanos Hotfixes are immediate :) Would be nice if can confirm fix.

@gkatsanos
Copy link

Confirmed.

@alvinindra
Copy link

image

Hello, are uuid already fix on new release?

@pi0
Copy link
Member Author

pi0 commented Dec 17, 2020

@alvinindra Quickly checking seems yes :) (https://runkit.com/pi0/5fdb46cd9a8519001ac7e9b5). You can upgrade using yarn upgrade nuxt and please make an issue with sandbox if not working.

@awronski
Copy link

@pi0 I just installed nuxt 2.14.12 and have problems with the urls handling.

Let's say I hava a url like this: http://127.0.0.1/unsunbscribe?email=some@email.com&token=DvtiwbIzry319e6KWimopA%3D%3D

    async asyncData ({ store, query, error }) {
      const email = query.email
      const token = query.token 
      [...]
    }

Nuxt should decode it to:

  • email: some@email.com - OK!
  • token: DvtiwbIzry319e6KWimopA== - ERROR

Instead I get DvtiwbIzry319e6KWimopA

Could you help?

Copy link
Member

@awronski Yes, I can replicate this behaviour - source and live example - would you be able to open a new issue so we can track?

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.

None yet

7 participants