Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fromExtensions and toExtensions translation when used with baseUrl #2969

Merged
merged 2 commits into from
Jun 22, 2020

Conversation

jknoxville
Copy link
Contributor

Motivation

First of all, I LOVE @slorber 's recent PR to add redirects - thank you so much for working on it! And I think the API is great. But I think I've found a small issue with it when combined with baseUrls other than '/'.

Using, fromExtensions, when you have a baseUrl of, for example /prefix/, the redirect file for the doc docs/somepath should be written to docs/somepath.html which would be a sibling of the real file: docs/somepath, but instead it is getting written to prefix/docs/somepath.html.

This means to hit the redirect, the baseUrl has to be included in the URL twice, so in this case the page itself would be accessible at /prefix/docs/somepath, and the html redirect would be accesible at /prefix/prefix/docs/somepath.html.

I've added a test case that I believe highlights what's happening, but you can repro it manually by setting a baseUrl of /prefix/, and then using fromExtensions: ['html'].

Observer that the redirect file gets written to/build/prefix/docs/page.html, rather than /build/docs/page.html.

I've also manually tested the following redirect:

redirects: [
          {
            from: '/docs/page.html',
            to: '/something/docs/page',
          },
        ],

and can confirm that this works as intended, so I think it's just the translation of fromExtensions that is processing the baseUrl wrong.

I've also made the same change in toExtensions, I think the same applies there too.

Solution

Since the translation works by mapping over all routes, and the routes already appear to have the baseUrl in them, I'm just removing that baseUrl when writing the "from" files.

Related PRs

#2793

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 19, 2020
@jknoxville jknoxville changed the title Fix fromExtensions and toExtensions when used with baseUrl fix: fromExtensions and toExtensions translation when used with baseUrl Jun 19, 2020
@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-2 ready!

Built with commit e14eadc

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

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Jun 19, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 0aa7011

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

@jknoxville
Copy link
Contributor Author

jknoxville commented Jun 19, 2020

Just doing some manual testing, I think I need to change the to: paths as well, seems the redirects are also including the baseUrl twice.

Making this a draft, it needs some more work. Sorry for the noise.

@jknoxville jknoxville marked this pull request as draft June 19, 2020 13:03
@slorber
Copy link
Collaborator

slorber commented Jun 19, 2020

Thanks @jknoxville :)

Yeah I did not think about the base Url 😅

Tell me when it's ready for review, or if you need help

@jknoxville
Copy link
Contributor Author

Ok, I've updated it now and it works when using yarn build. Testing steps:

First change the baseUrl in docusuarus.config.js to '/some/prefix/', then:

yarn
cd website
yarn build
mkdir -p some/prefix
cp -r build/* some/prefix/
python -m SimpleHTTPServer 80

Browse to localhost:80/some/prefix/docs/configuration.html

It works.

However, here's the weird bit:

yarn
cd website
yarn start

Browse to localhost:3000/some/prefix/docs/configuration.html

This doesn't work for some reason, even though if you strip the '.html', it does.

I haven't looked into how yarn start works yet, does anyone have any ideas why it would work with yarn build but not with yarn start?

@jknoxville jknoxville marked this pull request as ready for review June 22, 2020 14:15
@slorber
Copy link
Collaborator

slorber commented Jun 22, 2020

Ah yeah we should probably document that better. The plugin only works for a production build, by outputting some additional html files.
I think we could make it work for yarn start too, but it's not the case right now.

@slorber
Copy link
Collaborator

slorber commented Jun 22, 2020

I tried with baseUrl: '/build' without the trailing slash and had this error:

  • {"from":"//docs/next/docusaurus.config.js.html","to":"//docs/next/docusaurus.config.js"} => Validation error: to is not a valid pathname. Pathname should start with / and not contain any domain or query string

But it's not your fault anyway, so I'll merge this and we'll fix the other issue here: #2978

@slorber slorber merged commit ee5e59f into facebook:master Jun 22, 2020
@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Jun 22, 2020
@slorber
Copy link
Collaborator

slorber commented Jun 22, 2020

Hey @jknoxville

I'm thinking about it, and I think this whole plugin should rather be baseUrl insensitive.

For example, if I use baseUrl: '/build/', on D2 website.

The redirect I setup here will break, because all paths provided to the function will include the /build prefix.

  plugins: [
    [
      '@docusaurus/plugin-client-redirects',
      {
        fromExtensions: ['html'],
        createRedirects: function (path) {

          // redirect to /docs from /docs/intro,
          if (path === "/docs/intro") {
            return ["/docs"];
          }

        },
      },
    ],

This forces the user to replace "/docs/intro" by "/build/docs/intro"

Wonder if it's not better to actually remove all baseurls ahead of time. What do you think?

@jknoxville
Copy link
Contributor Author

Good point @slorber I think that makes a lot of sense.

My main concern was just getting from and to Extensions to work, since there's no doubt that they should be baseUrl independent, but I didn't think about the other.

It makes sense that you should be able to swap in any baseUrl, and any existing redirect rules will "just work" that would really be great in my opinion.

The only thing that would give me slight hesitation is when you consider that a docusaurus site can live along side statically served HTML (generated without docusaurus, e.g Javadocs, appledoc etc). These files might not necessarily have the same baseUrl as the docusaurus files. It's conceivable that people would want to setup redirection rules to those URLs.

But to be honest I think that seems like a rare case, and falls a bit out of the scope of this plugin IMO. It exists to redirect to docusaurus pages, not really to arbitrary paths, and if need be, the ability to redirect to paths without the baseUrl could always be added later with an option.

@slorber
Copy link
Collaborator

slorber commented Jun 23, 2020

Yes, anyway I don't think this plugin is suitable to write redirects from outside of its build folder. If you have /d2-site + /jekyll-site , I don't think the plugin should write redirect html files to /jekyll-site anyway.

Also, I think we could work on make this plugin work even in dev with yarn start, and if we start to do such redirects, that would be impossible to add later.

I've added a new issue here: #2982

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants