Adding baseUrl to feed links.

[skratchdot]

Motivation

This hopefully fixes a bug reported in jest:
jestjs/jest#6418

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

I considered adding a test for this, but there didn't appear to be any for this file. I can add them if you would like. For the time being, all I did was use the following as a smoke test:

• view-source:https://facebook.github.io/jest/blog/
• confirm og:image url works: https://facebook.github.io/jest/img/opengraph.png
• confirm atom.xml url is incorrect: https://facebook.github.io/blog/atom.xml
• after this pr, it should match: https://facebook.github.io/jest/blog/atom.xml

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

[docusaurus-bot]

Deploy preview for docusaurus-preview ready!

Built with commit 2505d4a

https://deploy-preview-745--docusaurus-preview.netlify.com

[skratchdot]

Actually, perhaps I should add a test for this. I think this PR will add 2 slashes when baseUrl is /jest/. I mean, that should work/resolve, but it still isn't 100% correct.

[skratchdot]

Yeah, the https://deploy-preview-745--docusaurus-preview.netlify.com/ atom.xml link has a double slash. So I feel like even though that "works", it's not good enough. Not sure if it's too much overhead to add some "url normalization" or if that's just a maintenance nightmare (and opens a can of worms). I could add some smarter string concatenation here (like checking whether to use /blog/atom.xml or blog/atom.xml), or maybe just using blog/atom.xml is good enough.

[endiliey]

You can just use a regex to replace the ending slash and still use /blog/atom.xml

[skratchdot]

@endiliey - good call. I'm just going to use a variable for this:

    // ensure the feedBase variable does not end in a slash
    const feedBase = (
      this.props.config.url + this.props.config.baseUrl
    ).replace(/\/$/, '');

[skratchdot]

I made a mountain out of a molehill, but I think it works now.

[JoelMarcey]

@skratchdot Nice! Maybe we can expand the use of your variable throughout the rest of the file? For example, I bet you would run into the same problem with twitter:image on line 56. Agree?

Also, I wonder if another alternative could have been using path.join() for these?

[endiliey]

@JoelMarcey I think using path.join for href is not a good idea. It will resolve to \\ in Windows. It's only good for use case like reading files, url are expected to be /

[JoelMarcey]

I always forget about Windows 🤣

[JoelMarcey]

@skratchdot Btw, If you prefer just to leave the PR as-is, I can make the additional updates to use your new variable as well in another PR.

[skratchdot]

I'm happy to make a change, but was worried about the other places since the final string concatenation part of those instances are variables. For instance: this.props.config.ogImage and this.props.config.twitterImage, instead of the hard-coded /blog/atom.xml and /blog/feed.xml. So I could see a similar issue popping up if I reuse the same variable (potentially a missing / if people don't configure their ogImage with a leading slash).

Also, I had considered using something like node's path or url, to do some normalization, but there are so many edge cases, and I wasn't sure how many other parts of the code would benefit (or if there are already some utility functions like that being used elsewhere).

WIth all that said, I'm happy with the PR as is, but am also willing to make changes (as long as they don't introduce too much risk).

[JoelMarcey]

I wonder if we should use a package like this? https://www.npmjs.com/package/url-join instead have a built-in package take care of it for us?

I'm happy to make a change, but was worried about the other places since the final string concatenation part of those instances are variables. For instance: this.props.config.ogImage and this.props.config.twitterImage, instead of the hard-coded /blog/atom.xml and /blog/feed.xml. So I could see a similar issue popping up if I reuse the same variable (potentially a missing / if people don't configure their ogImage with a leading slash).

Assuming we don't use a package, you could manually add the slash in the + concatenation, and then your regex would take it out if one already existed. Right?

[endiliey]

Just checked url-join, it's quite handy. Leaving it to @JoelMarcey & @yangshun if we want to use such package.

From my understanding, we always expect baseUrl to end with a slash actually. There's a lot of other codes that will break if it's not.

Maybe let's use something like websiteUrl or siteUrl instead of feedBase to promote reusability & common name.
E.g: siteUrl = https://facebook.github.io/jest/

[skratchdot]

I played with url-join via https://npm.runkit.com/url-join and it is nice. I tried some weird examples like:

var urljoin = require("url-join");
console.log(urljoin('//foo.example/', '///a//', '/b/cd', 'e', 'f', '?bar=123'));

and it seemed to work. The only worry I would have is the additional overhead/bytes of a new package (but the source doesn't seem that big).

I agree, if we start using the variable for more, then it should be renamed to something like siteUrl.

3 other thoughts:

1. Potentially there could be a siteConfig.js validation step before building the site that would check things, and error if criteria isn't met:

• baseUrl must end with a slash
• headerIcon must not start with a slash
• etc
  note: if we went this route, then the docs would need to be updated with examples/expected values

2. I think we can "fix" this specific PR by making sure feedBase/siteUrl always ends with a slash, and remove the leading slash from the 2 hardcoded /blog/*.xml links.
3. It would help to get some stats on some of the configs that are being used in the wild, and then create some unit tests based on those values (i.e. what do most baseUrl's look like? are there edge cases to deal with? if so, can we break those users in a semver major update assuming the messaging is clear?)

Would you all be okay with me trying to "fix" this PR by using the method I mentioned in my "thought # 2" above? Then we could address other issues/thoughts in a separate PR.

[endiliey]

I'm ok with your thought #2.

Most baseUrl always end with slash (never seen one without)

Reason-react
https://github.com/reasonml/reason-react/blob/de0803fa5784e434e3d9da374ec3e4e8a7653f12/website/siteConfig.js#L77

Jest
https://github.com/facebook/jest/blob/eb096f8b8d4a0db77fbce112fd758af7a6496764/website/siteConfig.js#L90

Loom
https://github.com/loomnetwork/loom-sdk-documentation/blob/ebe7f3ddda699613464b87801c28a16afcbb7227/website/siteConfig.js#L37

Even when user create new docusaurus project, we guided them to do so
https://github.com/facebook/Docusaurus/blob/33a0f309ff70c408bafa3912b86801a1856db745/examples/basics/siteConfig.js#L27-L30

[JoelMarcey]

@skratchdot Hi! In general, I am not too concerned about adding another package to use within the Docusaurus core. Any affect is really only seen in development -- since the output of Docusaurus is raw html -- and in this case, I don't think the development impact would be very large.

So I generally go with the question of whether the package will make Docusaurus better and more correct vs. the maintainers having to roll something themselves to do the same thing.

If we think url-join will allow us to not worry about URL formatting in the Docusaurus core, I think that is a benefit that should be highly considered.

[skratchdot]

@endiliey - Thanks for those examples! I just updated the code. I'll confirm the example site once it's rebuilt.

[endiliey]

This LGTM. I'll leave it to @JoelMarcey to approve.

Thanks for the PR, It made me realize we lack meta description tag which is crucial for SEO. Might file an issue for that 😄

[JoelMarcey]

I think I am good with this. Just one question inline.

[JoelMarcey]

So this won't cause a possible double ending // when we use it below?

For example, siteUrl always ends with a single slash. Could this.props.config.ogImage start with a single slash, causing a // in line 51?

Just wanting to make sure we are still good there.

[JoelMarcey]

I think even if the answer to my question was true, it should still work. Long term, maybe let's see about using url-join anyway.