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.

Sign up for GitHub

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(blog): apply baseUrl to relative image in blog authors #10440

Merged
merged 8 commits into from
Aug 29, 2024
+333 −10
Merged

fix(blog): apply baseUrl to relative image in blog authors #10440

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c9d04f7
fix: apply baseUrl to relative image
OzakIOne Aug 23, 2024
4f0214f
fix: baseUrl duplication
OzakIOne Aug 23, 2024
df86b3b
add tests
OzakIOne Aug 26, 2024
f18f124
move tests
OzakIOne Aug 27, 2024
3a69dd0
add docusaurus bug detection
OzakIOne Aug 27, 2024
bbc86e8
fix bug & add tests
OzakIOne Aug 27, 2024
bc3b714
Merge branch 'main' into ozaki/authorPageBaseURL
OzakIOne Aug 27, 2024
835afd4
Simplifies internal invariant check
slorber Aug 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion ...ges/docusaurus-plugin-content-blog/src/__tests__/__fixtures__/authorsMapFiles/authors.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

192 changes: 191 additions & 1 deletion packages/docusaurus-plugin-content-blog/src/__tests__/authors.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ describe('getBlogPostAuthors', () => {
{
OzakIOne marked this conversation as resolved.
Show resolved Hide resolved
key: 'slorber',
name: 'Sébastien Lorber',
imageURL: '/baseUrl/img/slorber.png',
imageURL: '/img/slorber.png',
page: null,
},
]);
Expand Down Expand Up @@ -522,3 +522,193 @@ describe('groupBlogPostsByAuthorKey', () => {
});
});
});

describe('getBlogPostAuthors baseUrl', () => {
// Global author without baseUrl
it('getBlogPostAuthors do not modify global authors imageUrl without baseUrl', async () => {
expect(
getBlogPostAuthors({
frontMatter: {
authors: ['ozaki'],
},
authorsMap: {
ozaki: {
name: 'ozaki',
key: 'ozaki',
imageURL: 'ozaki.png',
page: null,
},
},
baseUrl: '/',
}),
).toEqual([
{
imageURL: 'ozaki.png',
key: 'ozaki',
name: 'ozaki',
page: null,
},
]);
});

// Global author with baseUrl
it('getBlogPostAuthors do not modify global authors imageUrl with baseUrl', async () => {
expect(
getBlogPostAuthors({
frontMatter: {
authors: ['ozaki'],
},
authorsMap: {
ozaki: {
name: 'ozaki',
key: 'ozaki',
imageURL: 'ozaki.png',
page: null,
},
},
baseUrl: '/img/',
}),
).toEqual([
{
imageURL: 'ozaki.png',
key: 'ozaki',
name: 'ozaki',
page: null,
},
]);
});

// Inline author without baseUrl
it('getBlogPostAuthors can return imageURL without baseUrl for inline authors', async () => {
expect(
getBlogPostAuthors({
frontMatter: {
authors: [{name: 'ozaki', imageURL: 'ozaki.png'}],
},
authorsMap: undefined,
baseUrl: '/',
}),
).toEqual([
{
imageURL: 'ozaki.png',
key: null,
name: 'ozaki',
page: null,
},
]);
});

// Inline author with baseUrl
it('getBlogPostAuthors normalize imageURL with baseUrl for inline authors', async () => {
expect(
getBlogPostAuthors({
frontMatter: {
authors: [{name: 'ozaki', imageURL: '/ozaki.png'}],
},
authorsMap: undefined,
baseUrl: '/img/',
}),
).toEqual([
{
imageURL: '/img/ozaki.png',
key: null,
name: 'ozaki',
page: null,
},
]);
});

// Global author without baseUrl with a subfolder in img
it('getBlogPostAuthors do not modify globalAuthor imageUrl with subfolder without baseUrl', async () => {
expect(
getBlogPostAuthors({
frontMatter: {
authors: ['ozaki'],
},
authorsMap: {
ozaki: {
name: 'ozaki',
key: 'ozaki',
imageURL: 'img/ozaki.png',
page: null,
},
},
baseUrl: '/',
}),
).toEqual([
{
imageURL: 'img/ozaki.png',
key: 'ozaki',
name: 'ozaki',
page: null,
},
]);
});

// Global author with baseUrl with a subfolder in img
it('getBlogPostAuthors do not modify globalAuthor imageUrl with subfolder with baseUrl', async () => {
expect(
getBlogPostAuthors({
frontMatter: {
authors: ['ozaki'],
},
authorsMap: {
ozaki: {
name: 'ozaki',
key: 'ozaki',
imageURL: 'img/ozaki.png',
page: null,
},
},
baseUrl: '/img/',
}),
).toEqual([
{
imageURL: 'img/ozaki.png',
key: 'ozaki',
name: 'ozaki',
page: null,
},
]);
});

// Inline author without baseUrl with a subfolder in img
it('getBlogPostAuthors normalize imageURL from subfolder without baseUrl for inline authors', async () => {
expect(
getBlogPostAuthors({
frontMatter: {
authors: [{name: 'ozaki', imageURL: 'img/ozaki.png'}],
},
authorsMap: undefined,
baseUrl: '/',
}),
).toEqual([
{
imageURL: 'img/ozaki.png',
key: null,
name: 'ozaki',
page: null,
},
]);
});

// Inline author with baseUrl with a subfolder in img
it('getBlogPostAuthors normalize imageURL from subfolder with baseUrl for inline authors', async () => {
expect(
getBlogPostAuthors({
frontMatter: {
authors: [{name: 'ozaki', imageURL: '/img/ozaki.png'}],
},
authorsMap: undefined,
baseUrl: '/img/',
}),
).toEqual([
{
imageURL: '/img/img/ozaki.png',
key: null,
name: 'ozaki',
page: null,
},
]);
});
});
61 changes: 61 additions & 0 deletions packages/docusaurus-plugin-content-blog/src/__tests__/authorsMap.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ describe('getAuthorsMap', () => {
contentPaths,
authorsMapPath: 'authors.yml',
authorsBaseRoutePath: '/authors',
baseUrl: '/',
}),
).resolves.toBeDefined();
});
Expand All @@ -90,6 +91,7 @@ describe('getAuthorsMap', () => {
contentPaths,
authorsMapPath: 'authors.json',
authorsBaseRoutePath: '/authors',
baseUrl: '/',
}),
).resolves.toBeDefined();
});
Expand All @@ -100,6 +102,7 @@ describe('getAuthorsMap', () => {
contentPaths,
authorsMapPath: 'authors_does_not_exist.yml',
authorsBaseRoutePath: '/authors',
baseUrl: '/',
}),
).resolves.toBeUndefined();
});
Expand Down Expand Up @@ -305,3 +308,61 @@ describe('authors socials', () => {
expect(validateAuthorsMap(authorsMap)).toEqual(authorsMap);
});
});

describe('getAuthorsMap global author baseUrl', () => {
const fixturesDir = path.join(__dirname, '__fixtures__/authorsMapFiles');
const contentPaths = {
contentPathLocalized: fixturesDir,
contentPath: fixturesDir,
};

it('getAuthorsMap return imageURL with relative path', async () => {
const authorsMap = await getAuthorsMap({
contentPaths,
authorsMapPath: 'authors.yml',
authorsBaseRoutePath: '/authors',
baseUrl: '/',
});
expect(authorsMap?.ozaki?.imageURL).toBe('/ozaki.png');
});

it('getAuthorsMap normalize imageURL with baseUrl', async () => {
const authorsMap = await getAuthorsMap({
contentPaths,
authorsMapPath: 'authors.yml',
authorsBaseRoutePath: '/authors',
baseUrl: '/baseUrl/',
});
expect(authorsMap?.ozaki?.imageURL).toBe('/baseUrl/ozaki.png');
});

it('getAuthorsMap return imageURL with relative subdir path', async () => {
const authorsMap = await getAuthorsMap({
contentPaths,
authorsMapPath: 'authors.yml',
authorsBaseRoutePath: '/authors',
baseUrl: '/',
});
expect(authorsMap?.ozakione?.imageURL).toBe('/img/ozaki.png');
});

it('getAuthorsMap normalize imageURL with baseUrl and subdir same value', async () => {
const authorsMap = await getAuthorsMap({
contentPaths,
authorsMapPath: 'authors.yml',
authorsBaseRoutePath: '/authors',
baseUrl: '/img/',
});
expect(authorsMap?.ozakione?.imageURL).toBe('/img/img/ozaki.png');
});

it('getAuthorsMap normalize imageURL subdir with baseUrl', async () => {
const authorsMap = await getAuthorsMap({
contentPaths,
authorsMapPath: 'authors.yml',
authorsBaseRoutePath: '/authors',
baseUrl: '/blog/',
});
expect(authorsMap?.ozakione?.imageURL).toBe('/blog/img/ozaki.png');
});
});
8 changes: 5 additions & 3 deletions packages/docusaurus-plugin-content-blog/src/authors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@ type AuthorsParam = {
baseUrl: string;
};

function normalizeImageUrl({
export function normalizeImageUrl({
imageURL,
baseUrl,
}: {
imageURL: string | undefined;
baseUrl: string;
}) {
}): string | undefined {
return imageURL?.startsWith('/')
Copy link
Collaborator

@slorber slorber Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the goal is a double application, there's probably a better solution.

Remember that:

  • The user might use baseUrl /blog/
  • The user might put files in /static/blog/
  • An author image hosted at /blog/blog/authorImage.png is perfectly valid

I think I'd prefer to not have this extra condition

In the past (useBaseUrl hook) it has been annoying because some users will use baseUrl /docs/ or /blog/ and duplicate paths like /docs/docs/someImage.png or /blog/blog/someImage.png should be allowed

I'd like to have a unit test ensuring that we actually allow /baseUrl/baseUrl/img/ozaki.jpg

What we want is to not apply the baseUrl if the author is a global author (ie it has a key).

My suggestion:

  function toAuthor(frontMatterAuthor: BlogPostFrontMatterAuthor): Author {
    const author = {
      // Author def from authorsMap can be locally overridden by front matter
      ...getAuthorsMapAuthor(frontMatterAuthor.key),
      ...frontMatterAuthor,
    };

    return {
      ...author,
      key: author.key ?? null,
      page: author.page ?? null,
      // global author images have already been normalized
      imageURL: author.key ? author.imageURL : normalizeImageUrl({imageURL: author.imageURL, baseUrl}),
    };
  }

The unit tests should clearly cover this case

Copy link
Collaborator

@Josh-Cena Josh-Cena Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to have a unit test ensuring that we actually allow /baseUrl/baseUrl/img/ozaki.jpg

I don't think that's a good idea. No one should be producing these URLs on purpose. See my opinion in #6294 too.

If a URL is /image.png, it should be come /baseURL/image.png, but /baseURL/image.png should stay that way.

Copy link
Collaborator

@slorber slorber Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Josh-Cena it's important that urls are portable, and that we can later use React Router built-in basename which will not have this deduplication logic (although it won't apply to images but only links)

You'll see for yourself that existing sites have /docs/docs already, whether it makes sense to you or not: https://eclipse-muto.github.io/docs/docs/muto

I know you want devs to be explicit in this case and write links such as /docs/docs/muto, but this means this link is not "baseUrl" portable anymore.

Another case: the same sites at Meta are deployed in different fashions for public/internal usage, with/without baseUrl.

I'd like to discourage the usage of links that include the baseUrl to maximize portability.

? normalizeUrl([baseUrl, imageURL])
: imageURL;
Expand Down Expand Up @@ -122,7 +122,9 @@ ${Object.keys(authorsMap)
...author,
key: author.key ?? null,
page: author.page ?? null,
imageURL: normalizeImageUrl({imageURL: author.imageURL, baseUrl}),
imageURL: author.key
? author.imageURL
: normalizeImageUrl({imageURL: author.imageURL, baseUrl}),
OzakIOne marked this conversation as resolved.
Show resolved Hide resolved
};
}
}
Expand Down
9 changes: 8 additions & 1 deletion packages/docusaurus-plugin-content-blog/src/authorsMap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import _ from 'lodash';
import {readDataFile, normalizeUrl} from '@docusaurus/utils';
import {Joi, URISchema} from '@docusaurus/utils-validation';
import {AuthorSocialsSchema, normalizeSocials} from './authorsSocials';
import {normalizeImageUrl} from './authors';
import type {BlogContentPaths} from './types';
import type {
Author,
Expand Down Expand Up @@ -93,10 +94,12 @@ export function checkAuthorsMapPermalinkCollisions(
function normalizeAuthor({
authorsBaseRoutePath,
authorKey,
baseUrl,
author,
}: {
authorsBaseRoutePath: string;
authorKey: string;
baseUrl: string;
author: AuthorInput;
}): Author & {key: string} {
OzakIOne marked this conversation as resolved.
Show resolved Hide resolved
function getAuthorPage(): AuthorPage | null {
Expand All @@ -114,19 +117,22 @@ function normalizeAuthor({
...author,
key: authorKey,
page: getAuthorPage(),
imageURL: normalizeImageUrl({imageURL: author.imageURL, baseUrl}),
socials: author.socials ? normalizeSocials(author.socials) : undefined,
};
}

function normalizeAuthorsMap({
authorsBaseRoutePath,
authorsMapInput,
baseUrl,
}: {
authorsBaseRoutePath: string;
authorsMapInput: AuthorsMapInput;
baseUrl: string;
}): AuthorsMap {
return _.mapValues(authorsMapInput, (author, authorKey) => {
return normalizeAuthor({authorsBaseRoutePath, authorKey, author});
return normalizeAuthor({authorsBaseRoutePath, authorKey, author, baseUrl});
});
}

Expand All @@ -153,6 +159,7 @@ export async function getAuthorsMap(params: {
authorsMapPath: string;
authorsBaseRoutePath: string;
contentPaths: BlogContentPaths;
baseUrl: string;
}): Promise<AuthorsMap | undefined> {
const authorsMapInput = await getAuthorsMapInput(params);
if (!authorsMapInput) {
Expand Down
1 change: 1 addition & 0 deletions packages/docusaurus-plugin-content-blog/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ export default async function pluginContentBlog(
routeBasePath,
authorsBasePath,
]),
baseUrl,
});
checkAuthorsMapPermalinkCollisions(authorsMap);

Expand Down