webmasterish / vuepress-plugin-autometa

Auto meta tags plugin for VuePress 1.x
MIT License
40 stars 6 forks source link

Allow canonical_base to contain a path #6

Closed florimondmanca closed 4 years ago

florimondmanca commented 4 years ago

Description

Hi! Thanks for this plugin, very handy and powerful.

My use case is that I have a VuePress site served at a base URL path, for example https://example.com/blog. If I use https://example.com/blog as the canonical_base, URLs in twitter:url and og:url aren't correctly rendered. More specifically, a page whose path is /some/path is rendered as https://example.com/some/path, instead of https://example.com/blog/some/path.

This is because of how URL.resolve works:

> URL.resolve("https://example.com/blog", "/some/path")
"https://example.com/some/path"

This PR changes the resolution of the canonical URL to simply concatenating canonical_base with $page.path, taking care to never use two slashes in between.

Checklist

(I didn't check these since I didn't find the test files or guidelines on code style.)

florimondmanca commented 4 years ago

Hmm… I just saw that #3 basically proposed the opposite of these changes. @webmasterish What was the rationale for moving from path.join() (which I probably should have used here) to URL.resolve()?

webmasterish commented 4 years ago

Hi @florimondmanca

Thanks for contributing and for pointing out the issue.

To be honest, I can't even recall why I made the switch; I guess that's the price for being lazy and not adding comments.

I'll take a look and get back to you.

webmasterish commented 4 years ago

Reading comments here and there made me remember that using path.join() would cause the following issues:

I like the simplicity of your resolveURL(), but it needs to cover additional cases, so, it could be updated to something like the following:

const resolve_url = ( base, path ) => `${_.trimEnd( base, '/' )}/${_.trimStart( path, '/' )}`

But then again, there might be other cases where this function doesn't cover.

Another option that someone pointed out is url-join package which seems to do the job. I try avoiding adding packages unless it's really necessary, quite simply because I hate dealing with compatibility issues if authors update their packages or make breaking changes.

Let me know what you think.

florimondmanca commented 4 years ago

I like your suggestion on trimming both sides of the URL — I think it's the safest way to go. Also, url-join seems to do just this, but also a lot more we don't need. I'll update the PR. :)

webmasterish commented 4 years ago

Cool!

Let's hope we're not breaking anything.

Thanks

florimondmanca commented 4 years ago

Hi! Just popping by to confirm that the URLs are now correctly rendered in v0.1.13. :-) :+1:

webmasterish commented 4 years ago

Great!

Thanks