fix: error on reusing some of the reports #101
Conversation
* Custom Error class should be an Error inheritor * Initializing app from server should be checked for exceptions * Move from `node-targz` to an own version without `nextTick` (https://github.com/lafin/node-targz/blob/master/lib/index.js#L12)
`Array.prototype.find` in clean environment:
```
function find() { [native code] }
```
With dependencies and imports from `app.js`:
```
function (value, equals) {
equals = equals || this.contentEquals || Object.equals;
for (var index = 0; index < this.length; index++) {
if (index in this && equals(this[index], value)) {
return index;
}
}
return -1;
}
```
| throw new ReuseError(`Nothing to reuse in ${report}`, e); | ||
| } | ||
| }, | ||
| (e) => { |
There was a problem hiding this comment.
а почему именно такая запись, а не через catch ниже?
There was a problem hiding this comment.
Потому что это не равнозначный по поведению код.
Здесь два разных типа ошибки, которые нужно бросать.
Если ты напишешь catch ниже, то заодно схватишь ошибку Nothing to reuse in ....
There was a problem hiding this comment.
я думаю жека имел ввиду catch выше
| .pipe(zlib.createGunzip()) | ||
| .on('error', reject) | ||
| .pipe(tar.extract(destination)) | ||
| .on('error', reject) |
There was a problem hiding this comment.
достаточно ведь одной подписки на ошибки в самом конце. Если в каком-то из пайпов случится ошибка, то мы провалимся сразу в этот обработчик
| reuse && reuse.result.metaInfo && JSON.parse(reuse.result.metaInfo) | ||
| ); | ||
| let extraMeta = {}; | ||
| if (reuse && reuse.result.metaInfo) { |
There was a problem hiding this comment.
_.has(reuse, 'result.metaInfo')
There was a problem hiding this comment.
Вкусовщина в данном случае.
| } catch (e) {} | ||
| } | ||
|
|
||
| const metaInfoObj = Object.assign({}, state.metaInfo, extraMeta); |
There was a problem hiding this comment.
Object.assign умеет мерджить только top-level:
Object.assign({}, {a: {b: 1}}, {a: {z: 2}}); // ==> {a: {z: 2}}мы тут случайно не отгребем?
There was a problem hiding this comment.
Нет, не думаю.
@kvmamich?
There was a problem hiding this comment.
Когда metaInfo был JSON'ом, там был только один уровень
Теперь я вообще не знаю, что делать с этим кодом
Тут выполнялся мёрдж metaInfo из стейта и из реиспользуемого отчёта, но с тех пор, как в отчёте появился html...
There was a problem hiding this comment.
ну сейчас по коду мы просто не будем экстендить мету. Но мне кажется, что при реюзе пользователям вообще не нужна мета - они просто будут обновлять скрины
There was a problem hiding this comment.
Давайте вообще этот код уберем тогда? В нём как-то не осталось смысла.
There was a problem hiding this comment.
не, тут оставляем. Я заведу задачу про то, чтобы мету делать кликабельной уже на клиенте. И сюда будет приезжать валидный объект.
There was a problem hiding this comment.
Ок. Какие еще вопросы по этому PR?
There was a problem hiding this comment.
только про пайпы, но я про это отдельно почитаю ;)
There was a problem hiding this comment.
Ну а что там читать:
'use strict';
const path = require('path');
const os = require('os');
const homedir = os.homedir();
const decompress = require('./lib/utils').decompress;
return decompress(path.resolve(homedir, 'defunct.file'), path.resolve(homedir, 'test.txt'))
.then(() => console.log('success'))
.catch(e => console.error('error'));$ node test.js
errorУбираем строку номер 13 в lib/utils.js:
$ node test.js
events.js:182
throw er; // Unhandled 'error' event
^
Error: ENOENT: no such file or directory, open '/Users/flack/defunct.file'There was a problem hiding this comment.
да не, я говорил про
.pipe(zlib.createGunzip())
.on('error', reject)
в общем, забей :)
| it('should be an Error inheritor', () => { | ||
| const error = new ExtendableError('msg'); | ||
|
|
||
| assert.isTrue(error instanceof Error); |
There was a problem hiding this comment.
assert.instanceOf(new ExtendableError('msg'), Error);
There was a problem hiding this comment.
Хуже читается, кмк, но не принципиально.
| // tempDir at this moment contains only report archive and unpacked report in some directory | ||
| .then(() => { | ||
| const reportDirName = fs.readdirSync(tempDir) | ||
| .find((f) => fs.statSync(path.resolve(tempDir, f)).isDirectory()); | ||
| .filter((f) => fs.statSync(path.resolve(tempDir, f)).isDirectory())[0]; |
There was a problem hiding this comment.
вот тут я не понял. почему мы первый элемент берем?
There was a problem hiding this comment.
Потому что в директории две записи: gemini-report (директория) и свежескачанный тарник.
В оригинальном ревью @j0tunn говорил, что хардкодить имя директории отчета нельзя. Отсюда такое.
Первый мы берем, потому что это временная, нами созданная папка, там никаких других директорий быть не может.
Original problem and discussion: https://nda.ya.ru/3TjUi7
Commits are thoroughly described in theirs commit messages.