Skip to content

fix: do not push baseURL with the vue router#171

Merged
Baroshem merged 1 commit intonuxt-modules:chore/2.0.0-rc.1from
Lehoczky:fix/do-not-push-baseurl-twice-when-navigating
Sep 25, 2023
Merged

fix: do not push baseURL with the vue router#171
Baroshem merged 1 commit intonuxt-modules:chore/2.0.0-rc.1from
Lehoczky:fix/do-not-push-baseurl-twice-when-navigating

Conversation

@Lehoczky
Copy link
Copy Markdown
Contributor

@Lehoczky Lehoczky commented Sep 7, 2023

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

Fixes #166

For demo purposes I've implemented this change in my project and deployed the site here, so you can try it out with actual docsearch results.

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes (if not applicable, please state why)

@Baroshem
Copy link
Copy Markdown
Collaborator

Hey!

First of all, thanks for the PR!

I have a small concern that I wanted to talk to you about. This code you wrote certainly solves your issue you mentioned in a linked bug but I am not sure if it should be applied globally like this.

In your particular example, it resolves to a duplicated params in url, while I can think of examples where this wouldnt be a case.

For example, lets say that in your case, user would set baseUrl in nuxt.config.ts to something like v2 and then, in Algolia he would have the same structure as you have right now which is frontend-tools/somethin-else.

Taking into consideration this example, it could break that users code. Because DocSearch would point to frontend-tools/somethin-else without the v2 prefix because it would be removed by your code.

Maybe there is an another way where this functionality could be delivered to you without potentially causing issues to other cases? :)

@Lehoczky
Copy link
Copy Markdown
Contributor Author

Heey,

If the user changes the baseURL in their nuxt.config.ts, the whole site would need to be re-crawled by algolia, right? Since every pages' URL changed in that case. For example, if I changed the prefix to v2/frontend tools, I would re-crawl my site in algolia and then every item in my index would have the new base URL.

Is this the scenario you were thinking of?

@Lehoczky
Copy link
Copy Markdown
Contributor Author

Lehoczky commented Sep 10, 2023

I would also consider this as a bugfix, because now the links in the search results point to a different URL then they actually navigate to: if an anchor link has /baseURL/something as href, then it will navigate to /baseURL/baseURL/something instead.

If someone needs some additional logic for handling the routes, they can do so with transformItems, but I can't see the merit in pushing the baseURL twice to the router, it will certainly lead to unexpected 404 errors for anyone who is using GitHub Pages, GitLab Pages or just a domain where they don't deploy to root URL.

I'm trying to think of an edge cases here, where we might removing some part of the URL when we shouldn't, but the code really just eliminates the baseURL duplication. At least that's what I think it does:D

@Baroshem Baroshem changed the base branch from main to chore/2.0.0-rc.1 September 25, 2023 08:52
@Baroshem
Copy link
Copy Markdown
Collaborator

Changing the branch here from main to chore/2.0.0-rc.1

@Baroshem Baroshem merged commit 48b633d into nuxt-modules:chore/2.0.0-rc.1 Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

baseURL appended twice to the URL when clicking on search result

2 participants