-
Notifications
You must be signed in to change notification settings - Fork 2
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
Implement 'plugins-loader' #2
Conversation
@@ -0,0 +1,2 @@ | |||
language: node_js | |||
node_js: 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
покрытия не будет?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
не фапаю на такое
|
||
afterEach(() => sandbox.restore()); | ||
|
||
describe('.prototype', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
зачем такое название дескрайба? если ты везде будешь использовать такое именование, то про .only
можешь забыть - у тебя всегда будут пересекаться названия дескрайбов
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
поясни..
подразумевается, что в этом файле будет тестироваться только этот класс. only
всегда будет отрабатывать как надо.
если в другом файле будет такой же describe
, то родительский describe
будет другой и тоже все ок запустится
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
та мне не критично, уберу
assert.calledWith(plugin, 'some-tool'); | ||
}); | ||
|
||
it('should load plugins with `true` value in opts', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
этот и следующий тест перетащи вверх, чтобы тесты про should load
и should not load
были рядом
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
assert.isTrue(requirePlugin('some-plugin', 'prefix-')()); | ||
}); | ||
|
||
it('should throw if plugin with prefix contains a syntax error', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
почему syntax
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
у нас была бага в том, что когда из плагина лезла синтаксическая ошибка при require
, то мы неправильно ее отрабатывали, собственно и тест на это.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
давай обобщим и напишем просто error
:)
return utils.require(pluginNameWithPrefix); | ||
} catch (e) { | ||
if (isPluginNotFoundError(e, pluginNameWithPrefix)) { | ||
throw new Error(`Can not find plugin by paths '${pluginName}' or '${pluginNameWithPrefix}'`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
почему paths
? это ведь не пути, а названия модулей
load(plugins) { | ||
return _(plugins) | ||
.map((opts, name) => ({name, opts: opts === true ? {} : opts})) | ||
.reject((plugin) => plugin.opts === false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(plugin) => !plugin.opts
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
можно предположить, что конфигурация плагина может быть пустой строкой, тогда мы этой проверкой отбросим такой плагин, а это не ок
@sipayRT 🆙 |
const isPluginNotFoundError = (e, pluginName) => | ||
e.toString().includes(`Error: Cannot find module '${pluginName}'`); | ||
|
||
module.exports = (pluginName, prefix) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А не разбить ли эту штуку на несколько функций? Ведь по сути делается 2 раза одно и тоже. Сначала без префикса а потом с префиксом
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
вроде там не получится из-за цепочек try...catch
@@ -0,0 +1,44 @@ | |||
'use strict'; | |||
|
|||
const Loader = require('../../lib/loader'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А вот этот волшебный модуль app-module-path
, чтобы не писать ../../ ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
мне не нравится он :) а модуль не такой большой, чтобы ради двух рекваеров его ставить, как мне кажется :)
assert.calledWith(plugin, 'some-tool'); | ||
}); | ||
|
||
it('should load plugins with `true` value in opts', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
assert.throws(() => requirePlugin('some-plugin'), referenceError); | ||
}); | ||
|
||
it('should require plugin with prefix', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А мы не хотим проверять порядок?
Что сначала идет поиск плагина без префикса, а потом с префиксом?
cc @sipayRT @tormozz48 @j0tunn