Skip to content

Commit

Permalink
fix(v2): make client-redirect-plugin not baseUrl sensitive (#3010)
Browse files Browse the repository at this point in the history
* feat(v2): use relative routes path in postBuild hook

* feat(v2): use relativeRoutesPath in other methods and modify tests

* fix(v2): fix D2 client redirects and tests

* feat(v2): add tests for relativeRoutesPaths

* fix(v2): fix some typos in configValidation

* fix(v2): fix an eslint warning and restart github action

* refactor(v2): create a removePrefix method

* refactor(v2): inline unnecessary method
  • Loading branch information
teikjun committed Jun 29, 2020
1 parent b58a53e commit 2e055f4
Show file tree
Hide file tree
Showing 10 changed files with 81 additions and 71 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ import {removeTrailingSlash} from '@docusaurus/utils';

function createTestPluginContext(
options?: UserPluginOptions,
routesPaths: string[] = [],
relativeRoutesPaths: string[] = [],
): PluginContext {
return {
outDir: '/tmp',
baseUrl: 'https://docusaurus.io',
routesPaths,
relativeRoutesPaths,
options: normalizePluginOptions(options),
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,33 +15,32 @@ const createExtensionValidationTests = (
extensionRedirectCreatorFn: (
paths: string[],
extensions: string[],
baseUrl: string,
) => RedirectMetadata[],
) => {
test('should reject empty extensions', () => {
expect(() => {
extensionRedirectCreatorFn(['/'], ['.html'], '/');
extensionRedirectCreatorFn(['/'], ['.html']);
}).toThrowErrorMatchingInlineSnapshot(
`"Extension=['.html'] contains a . (dot) and is not allowed. If the redirect extension system is not good enough for your usecase, you can create redirects yourself with the 'createRedirects' plugin option."`,
);
});
test('should reject extensions with .', () => {
expect(() => {
extensionRedirectCreatorFn(['/'], ['.html'], '/');
extensionRedirectCreatorFn(['/'], ['.html']);
}).toThrowErrorMatchingInlineSnapshot(
`"Extension=['.html'] contains a . (dot) and is not allowed. If the redirect extension system is not good enough for your usecase, you can create redirects yourself with the 'createRedirects' plugin option."`,
);
});
test('should reject extensions with /', () => {
expect(() => {
extensionRedirectCreatorFn(['/'], ['ht/ml'], '/');
extensionRedirectCreatorFn(['/'], ['ht/ml']);
}).toThrowErrorMatchingInlineSnapshot(
`"Extension=['ht/ml'] contains a / and is not allowed. If the redirect extension system is not good enough for your usecase, you can create redirects yourself with the 'createRedirects' plugin option."`,
);
});
test('should reject extensions with illegal url char', () => {
expect(() => {
extensionRedirectCreatorFn(['/'], [','], '/');
extensionRedirectCreatorFn(['/'], [',']);
}).toThrowErrorMatchingInlineSnapshot(
`"Extension=[','] contains invalid uri characters. If the redirect extension system is not good enough for your usecase, you can create redirects yourself with the 'createRedirects' plugin option."`,
);
Expand All @@ -53,28 +52,28 @@ describe('createToExtensionsRedirects', () => {

test('should create redirects from html/htm extensions', () => {
const ext = ['html', 'htm'];
expect(createToExtensionsRedirects([''], ext, '/')).toEqual([]);
expect(createToExtensionsRedirects(['/'], ext, '/')).toEqual([]);
expect(createToExtensionsRedirects(['/abc.html'], ext, '/')).toEqual([
expect(createToExtensionsRedirects([''], ext)).toEqual([]);
expect(createToExtensionsRedirects(['/'], ext)).toEqual([]);
expect(createToExtensionsRedirects(['/abc.html'], ext)).toEqual([
{from: '/abc', to: '/abc.html'},
]);
expect(createToExtensionsRedirects(['/abc.htm'], ext, '/')).toEqual([
expect(createToExtensionsRedirects(['/abc.htm'], ext)).toEqual([
{from: '/abc', to: '/abc.htm'},
]);
expect(createToExtensionsRedirects(['/abc.xyz'], ext, '/')).toEqual([]);
expect(createToExtensionsRedirects(['/abc.xyz'], ext)).toEqual([]);
});

test('should create "to" redirects without baseUrl when baseUrl is used', () => {
test('should create "to" redirects when relativeRoutesPath contains a prefix', () => {
expect(
createToExtensionsRedirects(['/prefix/file.html'], ['html'], '/prefix/'),
).toEqual([{from: '/file', to: '/file.html'}]);
createToExtensionsRedirects(['/prefix/file.html'], ['html']),
).toEqual([{from: '/prefix/file', to: '/prefix/file.html'}]);
});

test('should not create redirection for an empty extension array', () => {
const ext: string[] = [];
expect(createToExtensionsRedirects([''], ext, '/')).toEqual([]);
expect(createToExtensionsRedirects(['/'], ext, '/')).toEqual([]);
expect(createToExtensionsRedirects(['/abc.html'], ext, '/')).toEqual([]);
expect(createToExtensionsRedirects([''], ext)).toEqual([]);
expect(createToExtensionsRedirects(['/'], ext)).toEqual([]);
expect(createToExtensionsRedirects(['/abc.html'], ext)).toEqual([]);
});
});

Expand All @@ -83,28 +82,28 @@ describe('createFromExtensionsRedirects', () => {

test('should create redirects to html/htm extensions', () => {
const ext = ['html', 'htm'];
expect(createFromExtensionsRedirects([''], ext, '/')).toEqual([]);
expect(createFromExtensionsRedirects(['/'], ext, '/')).toEqual([]);
expect(createFromExtensionsRedirects(['/abc'], ext, '/')).toEqual([
expect(createFromExtensionsRedirects([''], ext)).toEqual([]);
expect(createFromExtensionsRedirects(['/'], ext)).toEqual([]);
expect(createFromExtensionsRedirects(['/abc'], ext)).toEqual([
{from: '/abc.html', to: '/abc'},
{from: '/abc.htm', to: '/abc'},
]);
expect(createFromExtensionsRedirects(['/def.html'], ext, '/')).toEqual([]);
expect(createFromExtensionsRedirects(['/def/'], ext, '/')).toEqual([]);
expect(createFromExtensionsRedirects(['/def.html'], ext)).toEqual([]);
expect(createFromExtensionsRedirects(['/def/'], ext)).toEqual([]);
});

test('should create "from" redirects without baseUrl when baseUrl is used', () => {
expect(
createFromExtensionsRedirects(['/prefix/file'], ['html'], '/prefix/'),
).toEqual([{from: '/file.html', to: '/file'}]);
test('should create "from" redirects when relativeRoutesPath contains a prefix', () => {
expect(createFromExtensionsRedirects(['/prefix/file'], ['html'])).toEqual([
{from: '/prefix/file.html', to: '/prefix/file'},
]);
});

test('should not create redirection for an empty extension array', () => {
const ext: string[] = [];
expect(createFromExtensionsRedirects([''], ext, '/')).toEqual([]);
expect(createFromExtensionsRedirects(['/'], ext, '/')).toEqual([]);
expect(createFromExtensionsRedirects(['/abc'], ext, '/')).toEqual([]);
expect(createFromExtensionsRedirects(['/def.html'], ext, '/')).toEqual([]);
expect(createFromExtensionsRedirects(['/def/'], ext, '/')).toEqual([]);
expect(createFromExtensionsRedirects([''], ext)).toEqual([]);
expect(createFromExtensionsRedirects(['/'], ext)).toEqual([]);
expect(createFromExtensionsRedirects(['/abc'], ext)).toEqual([]);
expect(createFromExtensionsRedirects(['/def.html'], ext)).toEqual([]);
expect(createFromExtensionsRedirects(['/def/'], ext)).toEqual([]);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,7 @@ function validateCollectedRedirects(
);
}

const allowedToPaths = pluginContext.routesPaths.map((path) =>
path.replace(pluginContext.baseUrl, '/'),
);
const allowedToPaths = pluginContext.relativeRoutesPaths;
const toPaths = redirects.map((redirect) => redirect.to);
const illegalToPaths = difference(toPaths, allowedToPaths);
if (illegalToPaths.length > 0) {
Expand Down Expand Up @@ -91,7 +89,7 @@ It is not possible to redirect the same pathname to multiple destinations:

// We don't want to override an already existing route with a redirect file!
const redirectsOverridingExistingPath = redirects.filter((redirect) =>
pluginContext.routesPaths.includes(redirect.from),
pluginContext.relativeRoutesPaths.includes(redirect.from),
);
if (redirectsOverridingExistingPath.length > 0) {
console.error(
Expand All @@ -103,7 +101,7 @@ It is not possible to redirect the same pathname to multiple destinations:
);
}
redirects = redirects.filter(
(redirect) => !pluginContext.routesPaths.includes(redirect.from),
(redirect) => !pluginContext.relativeRoutesPaths.includes(redirect.from),
);

return redirects;
Expand All @@ -113,18 +111,16 @@ It is not possible to redirect the same pathname to multiple destinations:
function doCollectRedirects(pluginContext: PluginContext): RedirectMetadata[] {
return [
...createFromExtensionsRedirects(
pluginContext.routesPaths,
pluginContext.relativeRoutesPaths,
pluginContext.options.fromExtensions,
pluginContext.baseUrl,
),
...createToExtensionsRedirects(
pluginContext.routesPaths,
pluginContext.relativeRoutesPaths,
pluginContext.options.toExtensions,
pluginContext.baseUrl,
),
...createRedirectsOptionRedirects(pluginContext.options.redirects),
...createCreateRedirectsOptionRedirects(
pluginContext.routesPaths,
pluginContext.relativeRoutesPaths,
pluginContext.options.createRedirects,
),
];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ const addLeadingDot = (extension: string) => `.${extension}`;
export function createToExtensionsRedirects(
paths: string[],
extensions: string[],
baseUrl: string,
): RedirectMetadata[] {
extensions.forEach(validateExtension);

Expand All @@ -54,8 +53,8 @@ export function createToExtensionsRedirects(
if (extensionFound) {
const routePathWithoutExtension = removeSuffix(path, extensionFound);
return [routePathWithoutExtension].map((from) => ({
from: trimBaseUrl(from, baseUrl),
to: trimBaseUrl(path, baseUrl),
from,
to: path,
}));
}
return [];
Expand All @@ -68,7 +67,6 @@ export function createToExtensionsRedirects(
export function createFromExtensionsRedirects(
paths: string[],
extensions: string[],
baseUrl: string,
): RedirectMetadata[] {
extensions.forEach(validateExtension);

Expand All @@ -82,14 +80,10 @@ export function createFromExtensionsRedirects(
return [];
}
return extensions.map((ext) => ({
from: `${trimBaseUrl(path, baseUrl)}.${ext}`,
to: trimBaseUrl(path, baseUrl),
from: `${path}.${ext}`,
to: path,
}));
};

return flatten(paths.map(createPathRedirects));
}

function trimBaseUrl(path: string, baseUrl: string) {
return path.startsWith(baseUrl) ? path.replace(baseUrl, '/') : path;
}
5 changes: 4 additions & 1 deletion packages/docusaurus-plugin-client-redirects/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import writeRedirectFiles, {
toRedirectFilesMetadata,
RedirectFileMetadata,
} from './writeRedirectFiles';
import {removePrefix} from '@docusaurus/utils';

export default function pluginClientRedirectsPages(
_context: LoadContext,
Expand All @@ -25,7 +26,9 @@ export default function pluginClientRedirectsPages(
name: 'docusaurus-plugin-client-redirects',
async postBuild(props: Props) {
const pluginContext: PluginContext = {
routesPaths: props.routesPaths,
relativeRoutesPaths: props.routesPaths.map(
(path) => `/${removePrefix(path, props.baseUrl)}`,
),
baseUrl: props.baseUrl,
outDir: props.outDir,
options,
Expand Down
6 changes: 2 additions & 4 deletions packages/docusaurus-plugin-client-redirects/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,9 @@ export type RedirectOption = {
export type UserPluginOptions = Partial<PluginOptions>;

// The minimal infos the plugin needs to work
export type PluginContext = Pick<
Props,
'routesPaths' | 'outDir' | 'baseUrl'
> & {
export type PluginContext = Pick<Props, 'outDir' | 'baseUrl'> & {
options: PluginOptions;
relativeRoutesPaths: string[];
};

// In-memory representation of redirects we want: easier to test
Expand Down
16 changes: 16 additions & 0 deletions packages/docusaurus-utils/src/__tests__/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
addTrailingSlash,
removeTrailingSlash,
removeSuffix,
removePrefix,
getFilePathForRoutePath,
} from '../index';

Expand Down Expand Up @@ -419,6 +420,21 @@ describe('removeSuffix', () => {
});
});

describe('removePrefix', () => {
test('should no-op 1', () => {
expect(removePrefix('abcdef', 'ijk')).toEqual('abcdef');
});
test('should no-op 2', () => {
expect(removePrefix('abcdef', 'def')).toEqual('abcdef');
});
test('should no-op 3', () => {
expect(removePrefix('abcdef', '')).toEqual('abcdef');
});
test('should remove prefix', () => {
expect(removePrefix('abcdef', 'ab')).toEqual('cdef');
});
});

describe('getFilePathForRoutePath', () => {
test('works for /', () => {
expect(getFilePathForRoutePath('/')).toEqual('/index.html');
Expand Down
4 changes: 4 additions & 0 deletions packages/docusaurus-utils/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,10 @@ export function removeSuffix(str: string, suffix: string): string {
return str.endsWith(suffix) ? str.slice(0, -suffix.length) : str;
}

export function removePrefix(str: string, prefix: string): string {
return str.startsWith(prefix) ? str.slice(prefix.length) : str;
}

export function getFilePathForRoutePath(routePath: string): string {
const fileName = path.basename(routePath);
const filePath = path.dirname(routePath);
Expand Down
22 changes: 11 additions & 11 deletions packages/docusaurus/src/server/configValidation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,23 +90,23 @@ export function validateConfig(
abortEarly: false,
});
if (error) {
const unknownFields = error.details.reduce((formatedError, err) => {
const unknownFields = error.details.reduce((formattedError, err) => {
if (err.type === 'object.unknown') {
return `${formatedError}"${err.path}",`;
return `${formattedError}"${err.path}",`;
}
return formatedError;
return formattedError;
}, '');
let formatedError = error.details.reduce(
(accumalatedErr, err) =>
let formattedError = error.details.reduce(
(accumulatedErr, err) =>
err.type !== 'object.unknown'
? `${accumalatedErr}${err.message}\n`
: accumalatedErr,
? `${accumulatedErr}${err.message}\n`
: accumulatedErr,
'',
);
formatedError = unknownFields
? `${formatedError}These field(s) [${unknownFields}] are not recognized in ${CONFIG_FILE_NAME}.\nIf you still want these fields to be in your configuration, put them in the 'customFields' attribute.\nSee https://v2.docusaurus.io/docs/docusaurus.config.js/#customfields`
: formatedError;
throw new Error(formatedError);
formattedError = unknownFields
? `${formattedError}These field(s) [${unknownFields}] are not recognized in ${CONFIG_FILE_NAME}.\nIf you still want these fields to be in your configuration, put them in the 'customFields' attribute.\nSee https://v2.docusaurus.io/docs/docusaurus.config.js/#customfields`
: formattedError;
throw new Error(formattedError);
} else {
return value;
}
Expand Down
6 changes: 3 additions & 3 deletions website/docusaurus.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@
const versions = require('./versions.json');

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

module.exports = {
Expand Down

0 comments on commit 2e055f4

Please sign in to comment.