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

PROMO-251: OpenGraph dynamic image plugin prototype. #368

Merged
merged 17 commits into from
Dec 12, 2021

Conversation

Krakazybik
Copy link
Member

@Krakazybik Krakazybik commented Nov 20, 2021

CHANGELOG

#251
Добавлено: Прототип плагина динамической генерации og картиночек.
Как это работает?
Для манипуляций с изображениями используется sharp работающий через libvips. На этапе postBuild, когда у нас всё собрано, получаем инфу из doc плагина и на её основе генерируем изображение с необходимыми нам дополнительными слоями. Сами изображения и слои описываем в наших шаблонах. Если нам нужно применить конкретный шаблон для конкретного документа - используем правила.

  • Если при работе с документацией потребовалось использовать github-дискуссии, то стоит их прикрепить как see-also источники
  • Если PR связан с задачей, то необходимо проверить, что все требования по задаче выполнены
  • Перед тем, как отправлять изменения на ревью, нужно ознакомиться с CONTIBUTING-гайдлайнами
  • Перед тем, как отправлять изменения на ревью, нужно провести self-review своих изменений
  • Перед тем, как отправлять изменения на ревью, нужно дождаться CI-проверок
  • Перед тем, как отправлять изменения на ревью, нужно дать краткое описание изменений

@azinit azinit marked this pull request as draft November 20, 2021 16:26
@azinit azinit marked this pull request as ready for review November 27, 2021 19:55
Copy link
Member

@azinit azinit left a comment

Choose a reason for hiding this comment

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

@Krakazybik

  1. Верни пож ченджлог, который был в темплейте PR)
  2. Добавь пож комменты к диффу своему, чтобы было проще ревьювить и предлагать изменения
    (оч хочется втянуть, но многие вещи не оч интуитивно понятны - лучше бы дополнить их комментарием, например как тут)

Как все сделаешь - пингани еще раз как можно будет ревьювить))

Думаю втянем сразу же, только с небольшими правками, чтобы прод не развалить

Copy link
Member

@azinit azinit left a comment

Choose a reason for hiding this comment

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

.

Copy link
Member Author

@Krakazybik Krakazybik left a comment

Choose a reason for hiding this comment

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

Добавил комментарии по PR.

images.set(`${item.name}_${item.params.image}`, createImageFromTemplate(item));
}
});
return images;
Copy link
Member Author

Choose a reason for hiding this comment

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

Создание мапы уникальных sharp pipeline'ов для наших картинок. (проверка на уникальность пока бесполезна, т.к. в layout кроме текста ничего добавлять пока что нельзя, и у нас по сути нет других картинок в шаблоне).

// TODO: File not found exception?
const fonts = createFontsMapFromTemplates(templates);
const images = createImagesMapFromTemplates(templates);

Copy link
Member Author

Choose a reason for hiding this comment

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

Создаём коллекции шрифтов и картинок, которые в дальнейшем будем клонить и потом с ними работать.

website/open-graph-templates/basic/template.json Outdated Show resolved Hide resolved
website/open-graph-templates/config.json Outdated Show resolved Hide resolved
console.error("Config validation error");
return;
}
return config;
Copy link
Member Author

Choose a reason for hiding this comment

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

Чтение и валидация нашего основного конфига плагина.

website/src/shared/lib/open-graph-preview/index.js Outdated Show resolved Hide resolved
website/src/theme/DocItem/index.js Outdated Show resolved Hide resolved
website/src/theme/DocItem/index.js Outdated Show resolved Hide resolved
website/src/theme/DocItem/index.js Outdated Show resolved Hide resolved
@azinit azinit linked an issue Dec 4, 2021 that may be closed by this pull request
7 tasks
@azinit azinit changed the title OpenGraph dynamic image plugin prototype. PROMO-251: OpenGraph dynamic image plugin prototype. Dec 4, 2021
Copy link
Member

@azinit azinit left a comment

Choose a reason for hiding this comment

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

Таки посмотрел... Выглядит прям монструозно, крутецкую работу проделал!

Вопросы такие:

  1. Можно ли как-то оставить в плагине только критичный функционал, чтоб без проблем затащить что есть?
    (а то по многим модулям там есть вопросы, и кажется, что расширять функционал надо бы позднее, а не прям щас - сложно пока даже умом объять. И кажется именно из-за сильно огромного функционала влитие PR может затянуться. Если можно оставить только базис - я бы оставил действительно только его на данный момент .А остальное след. итерациями)

  2. Можно ли локально при билде - получить OG изображения файлов?
    (как я понимаю - изображения сгенерятся при билде, и потом по хешу, что будет в head.meta.og:image можно будет перейти по ссылке и получить статическое превью - верно?)

website/.eslintrc.js Outdated Show resolved Hide resolved
website/docusaurus.config.js Outdated Show resolved Hide resolved
website/docusaurus.config.js Outdated Show resolved Hide resolved
website/open-graph-templates/config.json Outdated Show resolved Hide resolved
website/open-graph-templates/config.json Outdated Show resolved Hide resolved
const layers = layout.map((item) => {
if (!Object.prototype.hasOwnProperty.call(doc, item.name)) {
console.error(`Wrong template config? Doc property ${item.name} not found.`);
return undefined;
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: Можно и проще, как ниже делал)

Suggested change
return undefined;
return;

Copy link
Member Author

Choose a reason for hiding this comment

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

Вернул, т.к. снова наткнулся на то что матюгальник проснулся =))
image_2021-12-10_23-42-02

if (layers.includes(undefined)) return;

return layers;
}
Copy link
Member

Choose a reason for hiding this comment

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

question: А для понимания еще раз - что все таки собой эти слои представляют?)

website/src/theme/DocItem/index.js Outdated Show resolved Hide resolved
website/src/theme/DocItem/index.js Outdated Show resolved Hide resolved
@Krakazybik
Copy link
Member Author

Krakazybik commented Dec 4, 2021

Таки посмотрел... Выглядит прям монструозно, крутецкую работу проделал!

Вопросы такие:

  1. Можно ли как-то оставить в плагине только критичный функционал, чтоб без проблем затащить что есть?
    (а то по многим модулям там есть вопросы, и кажется, что расширять функционал надо бы позднее, а не прям щас - сложно пока даже умом объять. И кажется именно из-за сильно огромного функционала влитие PR может затянуться. Если можно оставить только базис - я бы оставил действительно только его на данный момент .А остальное след. итерациями)
  2. Можно ли локально при билде - получить OG изображения файлов?
    (как я понимаю - изображения сгенерятся при билде, и потом по хешу, что будет в head.meta.og:image можно будет перейти по ссылке и получить статическое превью - верно?)
  1. На самом деле старался максимально изолированно делать всё, потому и делал через плагин, даже если что-то в процессе билда пойдёт не так, сайт сбилдится(точнее оно уже будет собрано т.к. делаем на postBuild) и ни как на функционал сайта это влиять не должно. И по сути ни каких breaking changes тут не должно быть.
  2. Да, на этапе postBuild, когда уже всё собрано (meta пути для OG уже прописаны, которые берутся из id документа и его локали), мы генерим наши превьюшки, получаем обычные jpg изображение сложенные в папку assert для базовой локали и ${locale}/assert для остальных.

@azinit
Copy link
Member

azinit commented Dec 4, 2021

2. Да, на этапе postBuild, когда уже всё собрано (meta пути для OG уже прописаны, которые берутся из id документа и его локали), мы генерим наши превьюшки, получаем обычные jpg изображение сложенные в папку assert для базовой локали и ${locale}/assert для остальных.

Да, удалось отдежабиться - круто)

  1. На самом деле старался максимально изолированно делать всё, потому и делал через плагин, даже если что-то в процессе билда пойдёт не так, сайт сбилдится(точнее оно уже будет собрано т.к. делаем на postBuild) и ни как на функционал сайта это влиять не должно. И по сути ни каких breaking changes тут не должно быть.

Да, если локально глянуть - то так и выглядит, спасибо)
Просто внешне это не особо заметно 🤔
image

@azinit
Copy link
Member

azinit commented Dec 4, 2021

@Krakazybik пока надо лично мне еще минимум час-два убить, чтобы доревьювить по мелочам + подправить темплейт и стили
Так что до след выходных постараюсь что-то подготовить

Но если оч хочешь влить, предлагаю разбить опять же этот PR на несколько частей

Как минимум:

  • PR1: docusaurus-плагин, неподключенный (да, это основа диффа, зато точно ничего не поломает, и можно влить до того, как превьюшку добьем)
  • PR2: подключение плагина + настройка конфига и превью

Copy link
Member

@azinit azinit left a comment

Choose a reason for hiding this comment

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

Выглядет рабочеспособно)
Как вольем - приступим к некст этапу 🚀


const config = getConfig(templatesDir);
if (!config) return;

Copy link
Member

Choose a reason for hiding this comment

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

Дополню для других, что плагинах докузавра так и принято описывать инициализацию плагинов, так что тут все конвенционально +-)

const { id, title } = doc;

const hashFileName = sha1(id + locale);

Copy link
Member

Choose a reason for hiding this comment

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

suggestion(to-improve, non-blocking): А мб можно вынести функцию вне основной инициализации, чтоб не разбухала?

Или она на контекст основной функции завязана?

@@ -0,0 +1,23 @@
const sharp = require("sharp");

function createImagePipeline(file) {
Copy link
Member

Choose a reason for hiding this comment

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

suggestion(to-improve, non-blocking): Не крит, но здесь и далее по возможности бы добавил комменты к функциям

Т.к. малясь абстрактно выглядят)

Copy link
Member

@azinit azinit Dec 9, 2021

Choose a reason for hiding this comment

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

+ свои мысли по расширению функционала закинешь, чтобы точно не потерялось)

Copy link
Member Author

Choose a reason for hiding this comment

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

Комменты в коде, или здесь? =)

Copy link
Member

Choose a reason for hiding this comment

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

в коде

}

return font.getSVG(text, options);
}
Copy link
Member

Choose a reason for hiding this comment

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

В коде комментарием тоже бы описать, достаточно неочевидный момент)

Comment on lines +39 to +42
"sha1": "^1.1.1",
"sharp": "^0.29.3",
"superstruct": "^0.15.3",
"text-to-svg": "^3.1.5",
Copy link
Member

Choose a reason for hiding this comment

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

thoughts: Эх, вынести бы как-нибудь, чтобы зависимости только во время постбилда пригождались, и не лезли в основной билд 🤔

Но out-of-scope, мб другие что подскажут

Copy link
Member

Choose a reason for hiding this comment

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

@Krakazybik мб ты что тут подскажешь, ты ведь достаточно много плагинов докузавра наизучал за это время)

Copy link
Member Author

@Krakazybik Krakazybik Dec 10, 2021

Choose a reason for hiding this comment

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

думаю =)

Copy link
Member Author

Choose a reason for hiding this comment

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

либо монорепу (лул), либо "sharp" "superstruct" "text-to-svg": можно в dev кинуть =) и отучить линтер матюгаться по import/no-extraneous-dependencies для папки плагина =)

Copy link
Member

@azinit azinit Dec 10, 2021

Choose a reason for hiding this comment

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

Ну пока так оставим, мб @Postamentovich @GhostMayor что-то подскажут

(для контекста опять же - это docusaurus-plugin, который чисто во время сборки нужен; потом отдельно в npm опубликуем наверное)

fs.readFileSync(`${templatesDir}\\${templateName}\\template.json`, encode),
),
}));

Copy link
Member Author

Choose a reason for hiding this comment

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

Читаем наши шаблоны, согласно полученным папкам. И возвращаем массив объектов с прочитанными данными, названием шаблона и путем до него.

Copy link
Member

Choose a reason for hiding this comment

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

Тогда мб лучше в сам код занести)

@Krakazybik
Copy link
Member Author

Krakazybik commented Dec 10, 2021

Кот починиль!
Это => тут.
Зарезолвил. вот это (я про getTemplateImageId если что )

Ну и так немного навёл марафета =)

Это чтобы не потерялось, а то комментарии пропали =)

@azinit
Copy link
Member

azinit commented Dec 11, 2021

@feature-sliced/core @feature-sliced/contributors Гляньте пож)

Уже 3-ю неделю влить не можем

@azinit
Copy link
Member

azinit commented Dec 11, 2021

@all-contributors please add @Krakazybik for code, plugin, tool

@allcontributors
Copy link
Contributor

@azinit

I've put up a pull request to add @Krakazybik! 🎉

@feature-sliced feature-sliced deleted a comment from allcontributors bot Dec 11, 2021
Copy link
Member

@Postamentovich Postamentovich left a comment

Choose a reason for hiding this comment

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

Все ок, я только обратил бы внимание на работу с файловой системой.
Путь до конфига и ассетов лучше собирать через path
И лучше не использовать синхронные методы чтения.
Но так как это выполняется в постбилде, возможно это не очень актуально.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PROMO: Add dynamic generation of social preview by article (for specific content)
3 participants