nuxt-modules / algolia

🔎 Algolia module for Nuxt
https://algolia.nuxtjs.org/
MIT License
185 stars 32 forks source link

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

Closed Lehoczky closed 10 months ago

Lehoczky commented 10 months ago

Types of changes

Description

Fixes https://github.com/nuxt-modules/algolia/issues/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:

Baroshem commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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