alexeia / middle.messenger.praktikum.yandex Goto Github PK

Предлагаю избавиться от проверки method === 'GET' в общей функции request и перенести эту логику в тело метода get, т.к. она не нужна для всех остальных методов

Originally posted by @dmitrymorozoff in #15 (comment)

Чтобы постоянно не писать JSON.stringify. Можно работу с веб сокетом обернуть в отдельный класс по аналогии с api. И там создать свойство метод send, который оборачивал бы объекты в JSON.stringify

Originally posted by @dmitrymorozoff in #15 (comment)

Приватные методы нужно писать с модификатором доступа private, иначе по умолчанию все методы public

Originally posted by @dmitrymorozoff in #5 (comment)

С учетом того, что это учебный проект и тестов не очень много, запускать их перед каждым коммитом - это ок, но в больших продакшен проектах правильнее всего тесты запускать в CI. А в пре-коммит хук можно добавить проверку линтеров. Причем делать проверку линтеров всего проекта на каждый коммит, по сути, не имеет смысла, и можно использовать библиотеку lint-staged - она прогоняет через линтер только те файлы, которые были изменены.

Originally posted by @Andrushka36 in #29 (comment)

Для времени есть специальный тег time

Originally posted by @dmitrymorozoff in #5 (comment)

Привет. В целом у тебя достаточно хорошая работа. Есть недоработки, но в целом все в порядке. Ход мыслей верный.

Единственное в чате, после обновления страницы, сообщения пропадают. Хорошо бы, чтобы они оставались.

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

По заданиям из ТЗ все в порядке. Работу приму. надеюсь ты поправишь вот эти небольшие недочеты к следующему спринту)

Originally posted by @dmitrymorozoff in #15 (review)

Привет. В целом у тебя достаточно хорошая работа. Есть недоработки, но в целом все в порядке. Ход мыслей верный.

Единственное в чате, после обновления страницы, сообщения пропадают. Хорошо бы, чтобы они оставались.

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

По заданиям из ТЗ все в порядке. Работу приму. надеюсь ты поправишь вот эти небольшие недочеты к следующему спринту)

Originally posted by @dmitrymorozoff in #15 (review)

URL бекенда сообщений лучше перенести в .env к URL основного бекенда. Только я вот не очень понял, я вижу, что вы в классах, отвечающих за взаимодействие с бекендом, - RegisterApi, LoginApi и др - используете захардкоженные значения URL, а не из .env. Почему так?

Originally posted by @Andrushka36 in #29 (comment)

Лучше всегда употреблять оператор строго равенства, вместо сравнения с приведением типов. Результат строгого равенства легче предугадать

Originally posted by @dmitrymorozoff in #15 (comment)

Мутировать значение по ссылке, не очень хорошая практика. Лучше создать новый объект с новыми данными. Статья на эту тему https://habr.com/ru/company/ruvds/blog/346998/

Originally posted by @dmitrymorozoff in #15 (comment)

Чтобы везде не дублировать {credentials: true} набор базовых опций можно передавать при инициализации httpTransport, базовый урл тоже можно задать при инициализации

Originally posted by @dmitrymorozoff in #15 (comment)

Если здесь h2, то почему тег p?

Originally posted by @dmitrymorozoff in #5 (comment)

А почему вы здесь делаете npm i express, а не npm i?

Originally posted by @Andrushka36 in #29 (comment)

У вас не проходят автотесты. Сейчас ошибка связана с тем, что после установки зависимостей выполняется npm run start, то есть node server.js, при этом в проекте нет папки dist, в которую смотрит сервер. Правильнее будет в package.json start-скрипт задать как npm run prod && node server.js.

Регулярное выражение нужно записать в константу и дать ей понятное название, о том, что она делает. Чтобы проще было читать код.

Тем более регулярное выражение дублируется в нескольких методах

Originally posted by @dmitrymorozoff in #15 (comment)

У вас в проекте достаточно много ts-ignore. Я бы посоветовал их использовать только в исключительных ситуациях и писать код так, чтобы он удовлетворял typescript. Например, здесь typescript пишет, что у вас не используется переменная req в коллбеке, и ведь вы ее действительно не используете. Поэтому здесь будет правильнее написать вот так:

rest.get(url, (_, res, ctx) => {
    return res(ctx.status(200), ctx.body('test get'));
}),

Originally posted by @Andrushka36 in #29 (comment)

Здесь нужно воспользоваться стрелочными функциями, чтобы избавиться от self

Originally posted by @dmitrymorozoff in #5 (comment)

Зачем это? Метод сам должен вызваться из родительского класса.

Originally posted by @dmitrymorozoff in #15 (comment)

Очень длинная строка, лучше каждое новое поле в объекте на новой строке писать, так будет проще читать код

Originally posted by @dmitrymorozoff in #15 (comment)

Лучше задавать фиксированные версии зависимостям.

Если вы однажды решите, удалить package-lock.json файл и попытаться повторно установить зависимости с помощью команды npm install, npm установит последние версии зависимостей, что может привести к ошибкам.

Плюс никогда не знаешь в какой версии появляется та или иная уязвимость, поэтому крайне рекомендую использовать статичную версию

Originally posted by @dmitrymorozoff in #5 (comment)

Хорошая практика писать 1 класс в 1 файле. Route и Router лучше разбить на 2 файла)

Originally posted by @dmitrymorozoff in #15 (comment)

Вижу, что вы зафиксировали версии всех зависимостей. Это круто, но, если что, удобнее всего это делать с помощью соответствующей директивы в .npmrc.

Originally posted by @Andrushka36 in #29 (comment)

А смысл от пустого интерфейса?

Originally posted by @dmitrymorozoff in #5 (comment)

Вы выложили свой проект на Heroku? Если нет, то это нужно сделать. Если да, то нужно добавить в README.md ссылку. Ну, и в Прочее у вас там некоторые артефакты с предыдущего спринта остались.

JSON.parse может упасть с ошибкой, при парсинге данных. Лучше обернуть блок в try/catch

Originally posted by @dmitrymorozoff in #15 (comment)

Лучше не обращаться к нативному тегу, а задать для него отдельный класс.

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

Хорошая статься не эту тему.

Originally posted by @dmitrymorozoff in #5 (comment)

А смысл от этой функции, которая выполняет такую простую операцию?

Originally posted by @dmitrymorozoff in #15 (comment)

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

Получится, что-то такое:

abstract class Block<Props extends {}> {
    private element: HTMLElement;
    private tagName: string;

    protected props: Props;

    public constructor(tagName: string = 'div', propsAndChildren: Props) {
        this.tagName = tagName;
        this.props = propsAndChildren;
    }
}

type ButtonProps = {
    onClick: () => {};
};

class Button extends Block<ButtonProps> {
    public constructor(props: ButtonProps) {
        super('div', props);
    }

    public onClick() {
        this.props.onClick();
    }
}

Originally posted by @dmitrymorozoff in #5 (comment)

Это нужно почистить

Originally posted by @dmitrymorozoff in #5 (comment)

А я что-то не очень понял, вы делаете импорт картинки, но никуда ее не вставляете. Как это работает, или это не нужно, и можно удалить?

Originally posted by @Andrushka36 in #29 (comment)