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: ability to load local files through cli option "require" #797

Merged
merged 1 commit into from
Oct 20, 2023

Conversation

DudaGod
Copy link
Member

@DudaGod DudaGod commented Oct 11, 2023

Fixes - #517

import { exists } from "./fs";

export const requireModule = async <T = unknown>(modulePath: string): Promise<T> => {
const isModuleLocal = await exists(modulePath);
Copy link
Member Author

Choose a reason for hiding this comment

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

Если переданный путь существует на fs, то с помощью path.resolve привожу к абсолютному пути и реквайрю. Такой подход подсмотрел в mocha

@@ -45,15 +46,21 @@ module.exports = class HermioneFacade {

ipc.on(MASTER_INIT, ({ configPath, runtimeConfig } = {}) => {
try {
const promise = Promise.resolve();
Copy link
Member Author

Choose a reason for hiding this comment

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

Подписка на событие - не асинхронная операция, поэтому реализовал через докидывание промисов.

({ requireModule } = proxyquire.noCallThru().load("../../../src/utils/module", {
"./fs": { exists },
[nodeModulePath]: isNodeModuleRequired,
[absoluteLocalModulePath]: isLocalModuleRequired,
Copy link
Member Author

Choose a reason for hiding this comment

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

Не придумал как это по другому протестить. require нормально застабать невозможно (я не смог по крайней мере).

Copy link
Member

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.

Что имеешь ввиду под минимальной фикстурой? У меня это вроде бы тоже минимальная. Т.е. проверяю правильный require модуля из node_modules и локального модуля.

Copy link
Member

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.

ну ты здесь всё застабал и от реальной реализации осталась только структура твоих модулей

ну так это же unit-тесты. Я в них застабал все лишнее. И проверяю только логику самой функции. Т.е. корректный реквайр модуля из node_modules или реквайр файл по абсолютному пути.

Я вообще сомневаюсь, что этот тест что-то отловит, если ты что-то в логике поменяешь :)

Если логику изменить, то тест упадет. Я это проверял при написании.

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

Ну по факту я не стабаю require. Я говорю, что при require такого несуществующего модуля вызови мой стаб и дальше я смотрю был он вызван или нет.

Про создание модуля на fs и его require я тоже думал. Но непонятно как в этом кейсе выполнить ассерт и убедится, что было зареквайрено то что нужно. Поэтому использовал текущий подход.

@DudaGod DudaGod force-pushed the HERMIONE-1210.require_local_files branch from 81a0cd2 to cdd8772 Compare October 20, 2023 09:35
@DudaGod DudaGod merged commit 42ee492 into master Oct 20, 2023
2 checks passed
@DudaGod DudaGod deleted the HERMIONE-1210.require_local_files branch October 20, 2023 09:49
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.

2 participants