fix(v2): normalizeUrl edge cases

[ayshiff]

Motivation

This PR fixes edge cases for the normalizeUrl util function like the ones introduced in the unit tests.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

This PR fixes #3374 and undo #3377.

New unit tests added:

      {
        input: ['/', ''],
        output: '/',
      },
      {
        input: ['', '/'],
        output: '/',
      },
      {
        input: ['/'],
        output: '/',
      },
      {
        input: [''],
        output: '',
      }

The thing was that for each part of our rawUrls (except for the last one) we removed the / in:

component.replace(/[/]+$/, i < urls.length - 1 ? '' : ‘/‘)

I added a condition to check if the current element is a slash and if so we can continue the process.

Here is the new logic inside our for loop:

Another workaround would be to add a condition to the returned string like: return str === '' ? '/'  : str as I think this change target only an edge case.

NOTE: I am note sure about this solution ! It would be great if someone could give me their opinion on this 👍

Related PRs

This PR undo the #3377 changes.

[docusaurus-bot]

Deploy preview for docusaurus-2 ready!

Built without sensitive environment variables with commit 05d305d

https://deploy-preview-3427--docusaurus-2.netlify.app

[slorber]

Thanks @ayshiff . That seems to solve the edge cases.

However, you should not revert the changes in the other PR, I'd like to keep them, as I don't think it's a good idea to enable users to provide routeBasePath = '' and it also contains other useful refactors.

It would be hard for me to review this 🤪 , but I guess if we have tests and they all pass it should be good.

What about adding more fancy edge case tests? I'm thinking of things like:

{
        input: ['/', '',"hello","","/","/","","/","/world"],
        output: '/hello/world',
},

You see what I mean?

[ayshiff]

@slorber I understand ! I'll keep the changes in the other PR.
I will add more fancy edge case tests then 👍

[ayshiff]

I spotted and fixed two new edge cases:

{
  input: ['', '/hello'],
  expected: '/hello',
  received: 'hello'
},
{
  input: ['hello/', ''],
  expected: 'hello/',
  received: 'hello'
}

For the starting slash the problem was that for every element which has an index greater than 1, we removed the starting slashes. So if we have something like ['', '/test'] it build the url: test.

We now have a new variable hasStartingSlash to cover that case.

The same problem was present for the ending slash, and the new variable hasEndingSlash should fix it 👍

[lex111]

Seems legit.

[slorber]

Hi Will be back from holiday on Monday 28 and catch up with PRs/issues soon 😉 Le mer. 16 sept. 2020 à 08:50, Alexey Pyltsyn <[email protected]> a écrit :

…

***@***.**** approved this pull request. Seems legit. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3427 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFW6PUFQ4WRGKVUQAJ6GFDSGBU3TANCNFSM4RCVNSMQ> .

[slorber]

Thanks for your work @ayshiff , seems to be more reliable now ;)