feat(v2): docusaurus-plugin-client-redirects #2793

slorber · 2020-05-22T18:16:56Z

First draft of plugin to handle client side redirects

I tried to make it flexible, and testable (tests will come)

It should cover basic redirections with ease, like path.html -> path
It should also cover the Watchman case: path -> path.html

Not ready to run, but you can take a look and give me some early feedback @yangshun ?

docusaurus-bot · 2020-05-22T18:20:41Z

Deploy preview for docusaurus-2 ready!

Built with commit 6b50763

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

lex111

I think this is an awesome feature, but for a complete understanding, I would like to see tests or/and draft doc.
Although we can make some improvements now.

packages/docusaurus-plugin-client-redirects/src/createRedirectPageContent.ts

packages/docusaurus-plugin-client-redirects/src/index.ts

packages/docusaurus-plugin-client-redirects/src/redirectCreators.ts

packages/docusaurus-plugin-client-redirects/src/utils.ts

slorber · 2020-05-24T17:47:46Z

Thanks for the feedback @lex111

It's not ready and not tested, just wanted to let you see a preview of the API I'm crafting (actually didn't even run the code yet so it might have bugs)

Will handle your feedbacks tomorrow.

packages/docusaurus-plugin-client-redirects/src/createRedirectPageContent.ts

…resh header + tests

slorber · 2020-05-25T18:15:01Z

Hi @lex111

I've added the tests and made the code more robust, it's almost finished. Will finish tomorrow.
It still has some bugs to fixes, and the write side involving fs is not tested yet).

When this feature is complete, using the following (main usecase)

    [
      '@docusaurus/plugin-client-redirects',
      {
        fromExtensions: ['html'],
      },
    ],

Should redirect from /path.html to /path and write a /path.html/index.html file (or maybe just /path.html ?)

But it should also support the "Watchman" case (#2697) that wants .html urls being the canonical ones:

    [
      '@docusaurus/plugin-client-redirects',
      {
        toExtensions: ['html'],
      },
    ],

Should redirect from /path to /path.html and write a /path/index.html file

For power users, it's also possible to provide a function:

    [
      '@docusaurus/plugin-client-redirects',
      {
        createRedirects: (existingPath) => {
           return ["existingPath" + "/someFancyPathSuffix"];
        },
      },
    ],

What do you think of this API?

Also, a subtility is to make sure the user does not create "bad redirects" that would override existing html files, or having duplicate redirects. For now I just filter those, but maybe there's a good way to warn about those issues?

slorber · 2020-05-26T19:00:26Z

So I think it's working fine now, and has good test coverage.

I propose to dogfood on the docusaurus v2 to redirect /path.html to /path, so that we know it keeps working over time.

It's already in the PR: https://deploy-preview-2793--docusaurus-2.netlify.app/docs/next/installation.html

I'll try the plugin on the Watchman site now and see if I have any issue.

And I'll write the doc.

lex111 · 2020-06-03T17:54:32Z

I'm thinking on making the createRedirects function more consistent, what do you think of:

I don’t know what are the advantages of this approach?

By the way, after the merging of #2861, it would be nice to add redirects to v2 website. I wonder if it is possible to use regular expressions for this purpose (to cover versioned docs)?

I did something similar when I needed to correctly highlight active items in navbar, so I'm curious if redirects could be added this way (using regex).

slorber · 2020-06-03T17:58:02Z

Hi @lex111

Here are some updates:

a new redirects plugin option, as you requeted
improved documentation an example with the 4 plugin options used together
stricter validation: it's barely impossible for the user to use badly formatted options
improved error messages
non-blocking warnings in case some redirects get filtered for security (ie that would lead to FS files overwriting each thers)
did not change the createRedirects (cf comment above)

Here's the config deployed in the PR, so that we can test better on D2 website:

[
      '@docusaurus/plugin-client-redirects',
      {
        fromExtensions: ['html'],
        redirects: [
          {
            to: '/',
            from: [
              '/plugin-client-redirects-tests/toHome1',
              '/plugin-client-redirects-tests/toHome2',
            ],
          },
          {
            to: '/docs',
            from: '/plugin-client-redirects-tests/toDocs1',
          },
          {
            to: '/docs',
            from: '/plugin-client-redirects-tests/toDocs2',
          },
          {
            to: '/docs',
            from: '/plugin-client-redirects-tests/toHomeDuplicatePath',
          },
        ],
        createRedirects: function (existingPath) {
          if (existingPath === '/') {
            return [
              '/',
              '/docs',
              '/plugin-client-redirects-tests/toHome3',
              '/plugin-client-redirects-tests/toHome4',
              '/plugin-client-redirects-tests/toHomeDuplicatePath',
            ];
          }
        },
      },
    ],

The following redirects work:

Html extension redirects:

Other redirects:

(the weird url lowercasing is related to Netlify, that's annoying with caps in pathname, shouldn't happen on other hosts)

As the config creates twice the toHomeDuplicatePath, and tries to redirect from / and /docs (existing paths that should not be overwritten by a redirect file), the build outputs the following warnings:

Do you see other things to handle?

slorber · 2020-06-03T18:07:00Z

About the version thing, I thought about it, and agree that's something we could support.
What about adding this in a new PR? As this one starts to get fat and we are waiting for it to fix the Watchman sites redirects.

It's already possible to add version redirects with the custom createRedirects fn in userland.
But I agree this is a pattern that we should make it simpler to configure.
I think we should take some time to design the behavior of such feature.

lex111 · 2020-06-03T18:40:58Z

Honestly, this plugin really came out huge and strong I thought there would be much less code in order to implement it. We actually did not pay so much attention to error handling in our existing plugins. However, I can’t say that this is a bad approach, just before that we did not attach so much importance to this.

I actually trust in a high test coverage of this plugin, since mentally this plugin is not easy to understand (I mean me). But this is really cool, although maybe we could simplify some things, for example, is it really necessary to use Yup? Although it may be necessary to go this way in our other plugins. I mean, maybe we should try to reconsider the development process of our plugins so that they are like this redirect plugin -- a lot of tests, a thorough validation of errors, a lot of high-level abstractions... Apparently I got carried away 😄

Okay, tomorrow I'll see it again in action and probably we can already merge it into master branch.

P.S. But please add redirects for introduction path. This will also be an example for release notes.

slorber · 2020-06-04T09:15:11Z

Hi @lex111

Honestly, this plugin really came out huge and strong I thought there would be much less code in order to implement it.

That's true, initially thought it would be smaller 😅

The size comes from giving more control to the user, and giving shortcuts for common patterns, with the fromExtensions/toExtensions.

We actually did not pay so much attention to error handling in our existing plugins. However, I can’t say that this is a bad approach, just before that we did not attach so much importance to this.

for example, is it really necessary to use Yup?

Although it may be necessary to go this way in our other plugins. I mean, maybe we should try to reconsider the development process of our plugins so that they are like this redirect plugin -- a lot of tests, a thorough validation of errors, a lot of high-level abstractions...

That's true. I did more user input validation in this plugin than other plugins. Personally I'd rather validate all user inputs on all plugins, fail-fast, and give actionable error messages to the user whenever possible, as it would lead to less github issues on the long term.

If you feel it's too much, not worth it, and not the direction we want to take, I'm ok with that and could remove all these validations.

since mentally this plugin is not easy to understand (I mean me)

a lot of high-level abstractions

I agree that it's not so easy to reason about this plugin, there are more hidden subtilities than I thought. Myself I often get confused, particularly inverting the from/to of the plugin. Maybe it's not the correct terminology to use, and we could find simpler wordings (like sourcePath/targetPath or something?)

About the abstractions, I don't think there's a lot of them, the only thing I really abstracted is the "redirectCreator" concept, which is function(path): redirects, and permit to fit the user provided createRedirects function and the "shortcuts" fromExtensions / toExtensions under the same interface.

I can remove this abstraction if you feel it's too hard to reason about. That means I'll need to write less generic code here:

function doCollectRedirects(pluginContext: PluginContext): RedirectMetadata[] {
  const redirectsCreators: RedirectsCreator[] = buildRedirectCreators(
    pluginContext.options,
  );

  const optionsRedirects = collectPluginOptionRedirects(pluginContext);

  const redirectCreatorsRedirects = flatten(
    redirectsCreators.map((redirectCreator) => {
      return createRoutesPathsRedirects(redirectCreator, pluginContext);
    }),
  );

  return [...optionsRedirects, ...redirectCreatorsRedirects];
}

That would become something like:

function doCollectRedirects(pluginContext: PluginContext): RedirectMetadata[] {
  return [
    ...collectToExtensionsRedirects(pluginContext),
    ...collectFromExtensionsRedirects(pluginContext),
    ...collectPluginOptionRedirects(pluginContext),
    ...collectCreateRedirects(pluginContext),
  ];
}

I think you are right, that may be simpler to understand, and this abstraction may not be worth it.

Do you agree I should refactor?

slorber · 2020-06-04T12:49:46Z

packages/docusaurus-plugin-client-redirects/src/collectRedirects.ts

+      pluginContext.options.createRedirects,
+    ),
+  ];
+}


@lex111 after removal of createRedirects abstraction

slorber · 2020-06-04T12:52:05Z

@lex111

I've removed the "redirectsCreator" abstraction, that simplified a bit the code.

Do you see other things to change?

For the doc home redirects, blocked by #2879
Maybe we can merge and fix this redirection later if needed?

lex111

@yangshun can you review the docs?

yangshun

Thanks for this @slorber. Will leave it to you to self-merge.

website/docusaurus.config.js

slorber · 2020-06-10T15:23:55Z

@lex111 FYI I added a redirect from /introduction:


const versions = require('./versions.json');

const allDocHomesPaths = [
  '/docs',
  '/docs/next',
  ...versions.slice(1).map((version) => `/docs/${version}`),
];

    [
      '@docusaurus/plugin-client-redirects',
      {
        fromExtensions: ['html'],
        createRedirects: function (path) {
          // redirect to /docs from /docs/introduction,
          // as introduction has been made the home doc
          if (allDocHomesPaths.includes(path)) {
            return [`${path}/introduction`];
          }
        },
      },
    ],

slorber · 2020-06-10T15:39:25Z

Finally merged !

@yangshun @lex111 should we make a release or we wait for the DocSearch V3 PR to be merged?

I've seen it in the milestone here: https://github.com/facebook/docusaurus/milestone/9

lex111 · 2020-06-10T15:45:33Z

@slorber aaaah! Why didn’t you squash all the commits into one when merging PR? I mean, this is the default option.

Currently, in master branch 45 extra commits! 😱

slorber · 2020-06-10T15:48:51Z

oooh nooo :/ not used to this workflow

actually it's not the default option for me, will look to make it the default, sorry :/

lex111 · 2020-06-10T15:53:03Z

So what do we do to fix this?

slorber · 2020-06-10T16:24:27Z

@lex111 discussed with @yangshun and @JoelMarcey , it's not a big deal for this time and they enabled the option to only allow squash merge for the future

they prefere to not force push to master (there's a security)

sorry about that :/

slorber · 2020-06-10T17:16:56Z

If needed, I have a clean branch that could be force-pushed to master here: https://github.com/slorber/docusaurus/commits/master-backup

JoelMarcey · 2020-06-10T20:25:38Z

I don't believe this causes much of a technical issue, requiring us to revert or force push. It might just make the commit log / history / etc a bit less clean.

@lex111 Are you seeing issues either in this repo or your fork because of the merge commit (instead of the squash)?

feat(v2): docusaurus-plugin-client-redirects

fd47ca1

slorber requested a review from yangshun as a code owner May 22, 2020 18:16

facebook-github-bot added the CLA Signed Signed Facebook CLA label May 22, 2020

slorber marked this pull request as draft May 22, 2020 18:17

lex111 reviewed May 23, 2020

View reviewed changes

slorber commented May 24, 2020

View reviewed changes

packages/docusaurus-plugin-client-redirects/src/createRedirectPageContent.ts Outdated Show resolved Hide resolved

slorber added 9 commits May 25, 2020 16:03

add utils tests

fde4933

createRedirectPageContent: use eta template + encode URI for meta ref…

d1a162e

…resh header + tests

add tests for redirectCreators

92c2d08

add proper normalizePluginOptions + tests

2695548

split collectRedirects and writeRedirectFiles in separate files

9579ac1

stable collectRedirects

d8c163e

improve a bit collectRedirects

26beb00

fix writeRedirectFiles

e93b98a

fix template import issue

0c330df

slorber added 5 commits May 26, 2020 19:03

fix missing ensureDir call

9a73680

prettierignore: packages/docusaurus-plugin-client-redirects/lib/

5efbe90

fix annoying inline snapshot not working due to fmt/indentation issue

8c8adc5

add write files test

cb2b5a5

fix bad redirection url

9a21295

slorber marked this pull request as ready for review May 26, 2020 19:00

slorber requested a review from lex111 May 26, 2020 19:00

slorber and others added 4 commits May 26, 2020 21:03

add canonical url

cc1705e

Merge branch 'master' into feature/client-side-redirects

03f4100

update broken snapshot

8b6dc03

add doc for @docusaurus/plugin-client-redirects

d6afb44

slorber added 2 commits June 3, 2020 19:46

Fix non-strict validation error, allowing [] for string

89038de

snapshot update

e60c063

add type comments to createRedirects doc

f6d1b2b

slorber mentioned this pull request Jun 4, 2020

RFC: validate plugin options with Yup #2878

Closed

Merge branch 'master' into feature/client-side-redirects

a02f90c

slorber mentioned this pull request Jun 4, 2020

Docs homepage bugs #2879

Closed

slorber added 2 commits June 4, 2020 14:28

refactor: remove redirectCreator abstraction

06673ee

rename fromRoutePath/toRoutePath to simply from/to

0ac5a66

slorber commented Jun 4, 2020

View reviewed changes

lex111 approved these changes Jun 8, 2020

View reviewed changes

yangshun approved these changes Jun 10, 2020

View reviewed changes

website/docusaurus.config.js Outdated Show resolved Hide resolved

website/docusaurus.config.js Outdated Show resolved Hide resolved

slorber added 2 commits June 10, 2020 17:22

add introduction redirect

c964a1e

migrate useful helper functions to docusaurus-utils

6b50763

slorber merged commit 68a1bb1 into facebook:master Jun 10, 2020

slorber added the pr: new feature This PR adds a new API or behavior. label Jun 17, 2020

jknoxville mentioned this pull request Jun 19, 2020

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(v2): docusaurus-plugin-client-redirects #2793

feat(v2): docusaurus-plugin-client-redirects #2793

slorber commented May 22, 2020 •

edited

Loading

docusaurus-bot commented May 22, 2020 •

edited

Loading

lex111 left a comment

slorber commented May 24, 2020

slorber commented May 25, 2020 •

edited

Loading

slorber commented May 26, 2020

lex111 commented Jun 3, 2020

slorber commented Jun 3, 2020 •

edited

Loading

slorber commented Jun 3, 2020 •

edited

Loading

lex111 commented Jun 3, 2020

slorber commented Jun 4, 2020

slorber Jun 4, 2020

slorber commented Jun 4, 2020 •

edited

Loading

lex111 left a comment

yangshun left a comment

slorber commented Jun 10, 2020

slorber commented Jun 10, 2020

lex111 commented Jun 10, 2020

slorber commented Jun 10, 2020

lex111 commented Jun 10, 2020

slorber commented Jun 10, 2020

slorber commented Jun 10, 2020

JoelMarcey commented Jun 10, 2020

feat(v2): docusaurus-plugin-client-redirects #2793

feat(v2): docusaurus-plugin-client-redirects #2793

Conversation

slorber commented May 22, 2020 • edited Loading

docusaurus-bot commented May 22, 2020 • edited Loading

lex111 left a comment

Choose a reason for hiding this comment

slorber commented May 24, 2020

slorber commented May 25, 2020 • edited Loading

slorber commented May 26, 2020

lex111 commented Jun 3, 2020

slorber commented Jun 3, 2020 • edited Loading

slorber commented Jun 3, 2020 • edited Loading

lex111 commented Jun 3, 2020

slorber commented Jun 4, 2020

slorber Jun 4, 2020

Choose a reason for hiding this comment

slorber commented Jun 4, 2020 • edited Loading

lex111 left a comment

Choose a reason for hiding this comment

yangshun left a comment

Choose a reason for hiding this comment

slorber commented Jun 10, 2020

slorber commented Jun 10, 2020

lex111 commented Jun 10, 2020

slorber commented Jun 10, 2020

lex111 commented Jun 10, 2020

slorber commented Jun 10, 2020

slorber commented Jun 10, 2020

JoelMarcey commented Jun 10, 2020

slorber commented May 22, 2020 •

edited

Loading

docusaurus-bot commented May 22, 2020 •

edited

Loading

slorber commented May 25, 2020 •

edited

Loading

slorber commented Jun 3, 2020 •

edited

Loading

slorber commented Jun 3, 2020 •

edited

Loading

slorber commented Jun 4, 2020 •

edited

Loading