feat: add 'watch' mode #138

eGavr · 2018-06-05T13:45:30Z

j0tunn

Над тестами бы подумать, конечно. Но в целом ок

j0tunn · 2018-06-05T14:03:54Z

lib/micromatch/index.js

+        const lastArg = args[args.length - 1];
+
+        if (args.length > 1 && isPlainObject(lastArg)) {
+            Object.assign({}, lastArg, options);


и с полученным в итоге объектом ты ничего не делаешь

j0tunn · 2018-06-05T14:05:59Z

package.json

    "cp-file": "5.0.0",
    "glob": "7.1.2",
    "glob-stream": "6.1.0",
    "graceful-fs": "4.1.11",
+    "lodash.isplainobject": "^4.0.6",


нафиг оно нужно, подключай весь lodash и не парься. Это же не клиентский код

j0tunn · 2018-06-05T14:08:58Z

lib/streams/watch-stream.js

+        const micromatchOptions = { dot: options.dot };
+        const cwd = this._cwd = options.cwd;
+
+        const chokidar = proxyquire('chokidar', {


оставь коммент зачем мы его так странно подключаем

j0tunn · 2018-06-05T14:11:12Z

lib/streams/watch-stream.js

+    /**
+     * Must be implemented in a child class of stream.Readable
+     */
+    // eslint-disable-next-line class-methods-use-this


это же отключает правило про пустую строчку сразу после объявления метода? А зачем там пустая строка? Почему нельзя написать:

_read() {} _read() { }

j0tunn · 2018-06-05T14:12:54Z

lib/streams/watch-stream.js

+        const cwd = this._cwd = options.cwd;
+
+        const chokidar = proxyquire('chokidar', {
+            anymatch: proxyquire('anymatch', { micromatch: micromatch.bindOptions(micromatchOptions) })


разнеси по строчкам, ну сложно же читается

j0tunn · 2018-06-05T14:13:54Z

README.md

+
+Tartifacts will work in an observe mode which means that all files and directories will be added to a destination directory or archive as soon as they appear on a file system.
+
+Note: in order to stop tartifacts `SIGTERM` signal should be sent


здесь нужно написать, что процесс будет завершен корректно (с закрытием всех файловых дескрипторов), но на это нужно время

j0tunn · 2018-06-05T14:15:59Z

lib/streams/watch-stream.js

+        watcher.on(WatcherEvents.add, path => this._push(path));
+        options.nodir || watcher.on(WatcherEvents.addDir, path => this._push(path));
+
+        process.on(SIGINT, () => {


SIGTERM тоже нужно обрабатывать. И, да, при таком переопределении теряется стандартная логика про exitCode

blond

Спасибо! В целом решение с Watch-стримом нравится.

Написал вопросы по конкретным кейсам.

blond · 2018-06-06T08:00:14Z

lib/streams/watch-stream.js

+
+        // 'chokidar' does not provide the ability to pass 'dot' option to 'micromatch' directly,
+        // so we use 'proxyquire' to monkey patch 'micromatch' in order to call all its methods with this option
+        const chokidar = proxyquire('chokidar', {


Давай вынесем это в отдельный модуль (файл), чтобы в стриме просто его рекваерить.

Тогда на эту логику можно будет написать тест: вызывается micromatch с необходимыми опциями.

blond · 2018-06-06T08:03:41Z

lib/micromatch/index.js

+ * @param {object} options
+ * @returns {function}
+ */
+exports.bindOptions = (options) => {


Добавь, пожалуйста, тесты на этот модуль и напиши пару слов зачем он нужен.

А я в месте использования написал, что там за магия происходит

blond · 2018-06-06T08:15:51Z

lib/streams/watch-stream.js

+        watcher.on(WATCHER_EVENTS.add, path => this._push(path));
+        options.nodir || watcher.on(WATCHER_EVENTS.addDir, path => this._push(path));
+
+        SIGNALS.forEach((signal) => process.on(signal, () => this._close(watcher)));


Как это будет работать в CLI?

Нажал я такой cltr + c, что должно произойти?) Что я увижу как пользователь?

blond · 2018-06-06T08:18:12Z

lib/streams/watch-stream.js

+        });
+
+        const watcher = chokidar.watch(patterns, { cwd, followSymlinks: options.follow });
+        watcher.on(WATCHER_EVENTS.add, path => this._push(path));


Я правильно понимаю, что тут и файлы и симлинки?

Что с событиями удаления (unlink и unlinkDir)?

Если сработает change, что в итоге попадёт в архив? Первая версия этого файла? Файл вообще не попадёт?

Вот кейс который меня интересует. В ENB «таргеты» по смыслу являются кэшами.

Т.е. во время сборки не только добавляются новые файлы, но и старые могут быть изменены, а так же не изменены (если mtime подходит). Но эти файлы всё равно должны попасть в архив.

Как это будет работать с текущим решением?

j0tunn

Ну и jsdoc ты зря повыгашивал, наверное

j0tunn · 2018-06-08T12:49:18Z

lib/artifact/artifact.js

+
+module.exports = class Artifact {
+    static create(destDir, streams) {
+        return new Artifact(destDir, streams);


j0tunn · 2018-06-08T12:49:35Z

lib/artifact/artifact.js

+
+    write() {
+        return makeDir(this._destDir, { fs })
+            .then(() => {


async await?

j0tunn · 2018-06-08T12:51:27Z

lib/artifact/index.js

+const TarStream = streams.TarStream;
+const CopyStream = streams.CopyStream;
+const TransformStream = streams.TransformStream;
+const WatchStream = streams.WatchStream;


desctructuring

j0tunn · 2018-06-08T12:51:48Z

lib/artifact/index.js

+    };
+    const writeStreamOptions = {emptyFiles: options.emptyFiles, emptyDirs: options.emptyDirs};
+    const gzipOptions = {gzip: options.gzip, gzipOptions: options.gzipOptions};
+


лишняя пустая строка. А тут линтер не настроен?

он настроен, но не ругается

j0tunn · 2018-06-08T13:00:27Z

lib/artifact/artifact.js

+    write() {
+        return makeDir(this._destDir, { fs })
+            .then(() => {
+                return new Promise((resolve, reject) => {


этот промис нужно создавать до makeDir

eGavr · 2018-06-08T23:16:21Z

Тестов новых пока не писал

После рефакторинга все старые тесты проходят (я сами тесты не выгашивал, а просто подправил род новую архитектуру)

blond

Пару замечаний по документации.

Тестов конечно не хватает :)

Давай после влития pr выпустим препатч и проверим с помощью dobby, что в проектах всё продолжает работать как и раньше.

blond · 2018-06-13T09:07:22Z

README.md

+
+Tartifacts will work in an observe mode which means that all files and directories will be added to a destination directory or archive as soon as they appear on a file system.
+
+Note: in order to stop tartifacts `SIGTERM` or `SIGINT` signal should be sent; these signals stop observing of a file system, but do not abort the process, so all file descriptors will be closed correctly and all tasks will be finished after a while


Поправь документацию, сейчас нужно вызывать метод, а не отправить сигналы.

blond · 2018-06-13T09:09:14Z

README.md

+
+process.on('SIGTERM', () => tartifacts.closeArtifacts());
+
+tartifacts.writeArtifacts({


Надо в API документации добавить про класс Tartifacts и его методы.

добавил

eGavr · 2018-06-13T16:25:22Z

Давай после влития pr выпустим препатч и проверим с помощью dobby, что в проектах всё продолжает работать как и раньше.

проверил

Тестов конечно не хватает :)

постараюсь на новую функциональность дописать в свободное время

eGavr force-pushed the feat/watcher-mode branch 2 times, most recently from 3f3bbb2 to b85f4a4 Compare June 5, 2018 13:54

j0tunn approved these changes Jun 5, 2018

View reviewed changes

eGavr force-pushed the feat/watcher-mode branch 5 times, most recently from 81f3283 to e644d34 Compare June 5, 2018 14:51

blond reviewed Jun 6, 2018

View reviewed changes

eGavr force-pushed the feat/watcher-mode branch from e644d34 to 55b9947 Compare June 8, 2018 12:08

j0tunn requested changes Jun 8, 2018

View reviewed changes

eGavr force-pushed the feat/watcher-mode branch from 55b9947 to 9319402 Compare June 8, 2018 23:14

eGavr force-pushed the feat/watcher-mode branch 2 times, most recently from c88f7fb to 3f828c0 Compare June 9, 2018 12:43

blond approved these changes Jun 13, 2018

View reviewed changes

eGavr force-pushed the feat/watcher-mode branch 2 times, most recently from 18b7b72 to 1a9355e Compare June 13, 2018 14:05

eGavr added 2 commits June 13, 2018 17:05

feat: option 'watch'

21974c2

major: drop node 4 and 8

94c48c6

eGavr force-pushed the feat/watcher-mode branch from 1a9355e to 94c48c6 Compare June 13, 2018 14:05

eGavr merged commit 12d7249 into yandex:master Jun 13, 2018

eGavr deleted the feat/watcher-mode branch June 13, 2018 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add 'watch' mode #138

feat: add 'watch' mode #138

eGavr commented Jun 5, 2018

j0tunn left a comment

j0tunn Jun 5, 2018

j0tunn Jun 5, 2018

j0tunn Jun 5, 2018

j0tunn Jun 5, 2018

j0tunn Jun 5, 2018

j0tunn Jun 5, 2018

j0tunn Jun 5, 2018

blond left a comment

blond Jun 6, 2018

blond Jun 6, 2018

eGavr Jun 8, 2018

blond Jun 6, 2018

blond Jun 6, 2018

blond Jun 6, 2018

blond Jun 6, 2018

j0tunn left a comment

j0tunn Jun 8, 2018

j0tunn Jun 8, 2018

j0tunn Jun 8, 2018

j0tunn Jun 8, 2018

eGavr Jun 8, 2018

j0tunn Jun 8, 2018

eGavr commented Jun 8, 2018

blond left a comment

blond Jun 13, 2018

blond Jun 13, 2018

eGavr Jun 13, 2018

eGavr commented Jun 13, 2018


		Tartifacts will work in an observe mode which means that all files and directories will be added to a destination directory or archive as soon as they appear on a file system.

		Note: in order to stop tartifacts `SIGTERM` signal should be sent


		process.on('SIGTERM', () => tartifacts.closeArtifacts());

		tartifacts.writeArtifacts({

feat: add 'watch' mode #138

feat: add 'watch' mode #138

Conversation

eGavr commented Jun 5, 2018

j0tunn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blond left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j0tunn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eGavr commented Jun 8, 2018

blond left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eGavr commented Jun 13, 2018