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

Обновление grpc-playground #237

Closed
sollidy opened this issue Aug 7, 2023 · 21 comments · Fixed by #245
Closed

Обновление grpc-playground #237

sollidy opened this issue Aug 7, 2023 · 21 comments · Fixed by #245
Assignees

Comments

@sollidy
Copy link
Contributor

sollidy commented Aug 7, 2023

С чем связан запрос на фичу?

Обновление зависимостей в репозитории grpc-playground

Расскажите как вы это себе видите

После обновления зависимостей (минорных или мажорных версий) следующие шаги должны пройти без ошибок:

  • установка зависимостей: yarn (обратить внимание на peerDependencies, если нужно, добавить в yarnrc)
  • чек: yarn check
  • тесты: yarn test unit
  • build или prepack: опционально по наличию команды.

Приложите пример реализаций

@SlumberyDude
Copy link
Contributor

SlumberyDude commented Aug 8, 2023

Пытаюсь установить зависимости репозитория и проверить его работоспособность до того, как буду проводить обновления пакетов.

Пока что при запуске yarn натыкаюсь на ошибку

@apollo/protobufjs@npm:1.2.2 couldn't be built successfully

В логах по данной ошибке

Error [ERR_LOADER_CHAIN_INCOMPLETE]: "file:///Users/kolodinevgenij/projects/atls/nestjs/.pnp.loader.mjs 'resolve'" did not call the next hook in its chain and did not explicitly signal a short circuit. If this is intentional, include `shortCircuit: true` in the hook's return.

Как будто ошибка внутри .pnp.loader.mjs, но гугление позволяет понять, что проблема в версиях Node.Js / yarn Баг описан вот тут yarnpkg/berry#4778

Похоже, что нужно либо снизить версию ноды до 16.16 или старше (сейчас у меня 19.0.1), либо обновить yarn.

В репозитории лежит билд yarn yarnPath: .yarn/releases/yarn-0.0.1-git.20221107.hash-d21b699.cjs, который используется при установке зависимостей yarn install с которым появляется ошибка. При этом в package.json указано "packageManager": "yarn@3.3.0".

Если вероломно поставить версию в пакете в 3.3.0 выполнив команду yarn set version 3.3.0, то установка зависимостей с yarn install проходит нормально, но вносятся изменения в файлы конфигураций yarn вроде yarn.lock .yarnrc.yml

В связи с вышесказанным есть вопросы:

  • Пока что не понимаю, нужно ли в рамках данного таска обновлять версию yarn? Понижать версию node?

Попробовал установить зависимости с node.js 16.16.0, все работает. 16.17.0 уже падает с той же ошибкой.

  • Под какую версию node.js необходимо проводить обновление?

Последняя версия 20.5.0, значит ли это, что под нее? Тогда судя по всему придется обновить yarn, что так или иначе затронет весь репозиторий, а не только grpc-playground воркспейс.

  • Насколько изолированным должно быть обновление зависимостей?

grpc-playground зависит от других воркспейсов, я так понимаю их я не трогаю, а пытаюсь обновить версии всего остального в данном пакете.

@SlumberyDude
Copy link
Contributor

SlumberyDude commented Aug 8, 2023

Судя по схожей таске atls/common#6 сидеть на старой ноде (16.16.0) не вариант.

В таком случае встает вопрос с обновлением yarn в репозитории.

@sollidy
Copy link
Contributor Author

sollidy commented Aug 8, 2023

последняя наша версия ярна лежит здесь:

@sollidy
Copy link
Contributor Author

sollidy commented Aug 8, 2023

обновление ярна командой:

  • yarn set version ../tools/yarn/cli/bundles/yarn.js

@TorinAsakura
Copy link
Member

@sollidy уже обновил…щас закину

@SlumberyDude
Copy link
Contributor

Вижу апдейт. Солью изменения и буду продолжать.

@SlumberyDude
Copy link
Contributor

SlumberyDude commented Aug 8, 2023

Чтобы обновить одну из зависимостей get-port https://www.npmjs.com/package/get-port/v/7.0.0 до последней версии придется обновить typescript, вроде до мажорной 5.0.0 (там впервые встречаю такое использование ключевого слова type), иначе yarn check не проходит тайпчек модуля.

../../../.yarn/berry/cache/get-port-npm-7.0.0-72b8a92f99-9.zip/node_modules/get-port/index.d.ts
Error: Module '"node:net"' has no exported member 'type'.

Ругается конкретно на вот эту строчку из пакета

import {type ListenOptions} from 'node:net';

Кажется странным, что чекаются пакеты зависимостей, это ок?

Пока что остался на более ранней версии пакета get-port 6.1.2 с которой все чеки проходят.

Не совсем понимаю по peerDependencies.

  • Должны ли они соответствовать обновленным devDependencies или же должны соответствовать версиям зависимостей, которые используют воркспейсы включенные в dependencies, то есть @atls/nestjs-grpc-http-proxy и @atls/nestjs-grpc-reflection?

И еще,

  • Требуется ли что-то делать с примером, использующим модуль @examples/grpc-playground? Нужно ли убеждаться в его нормальной работе после обновления пакетов модуля @atls/nestjs-grpc-playground?

Там версия неста наверное не зайдет, я обновляю до самой свежей 10.1.3

Открыл PR
#243

Чет все интеграционные тесты упали

@sollidy
Copy link
Contributor Author

sollidy commented Aug 8, 2023

https://github.com/atls/hyperion/wiki/%D0%A7%D1%82%D0%BE-%D1%82%D0%B0%D0%BA%D0%BE%D0%B5-peerDependencies

@sollidy sollidy linked a pull request Aug 8, 2023 that will close this issue
@SlumberyDude
Copy link
Contributor

Почитал про peerDependencies

Я правильно понимаю, что мне нужно полностью их прокачать до последних версий (какие возможно), как я это сделал в devDependencies?

Это будет требовать от родительского пакета (от клиента) использования именно последней версии неста или выше ^10.1.3

Не правильнее ли зафиксировать только мажорную, как это было сделано до обновления (была ^8.0.0), то есть поставить ^10.0.0?

@TorinAsakura
Copy link
Member

@SlumberyDude Нет. Не правильно.

@SlumberyDude
Copy link
Contributor

SlumberyDude commented Aug 9, 2023

Нужно ли трогать модуль rxjs? Его версия 7.5.4 есть в resolutions в корневом package.json и вроде бы это не дает мне использовать в обновляемом модуле версию выше. Либо же фиксить resolutions, убирать из него rxjs например.

@TorinAsakura
Copy link
Member

@SlumberyDude Ты всегда должен пробовать грейдиться, чтобы, как минимум, проверить на потенциальные проблемы совместимости

@SlumberyDude
Copy link
Contributor

@TorinAsakura Если обновляю rxjs до последней 7.8.1 внутри grpc-playground, то все чеки и юниты проходят, но вебшторм указывает на несовпадение версий с той, которая есть в корневом resolutions.

Повышаю ее и у меня ситуация зеркалится, теперь уже вебшторм ругается на несовпадение версии в других пакетах. Чеки все еще проходят.

Также до конца не понял ответ на вчерашний вопрос. Повышаю до упора peerDependency или нет?

@SlumberyDude
Copy link
Contributor

SlumberyDude commented Aug 10, 2023

Мысли по поводу провала тайпчека в PR

После удаления зависимости rxjs 7.5.4 из resolutions возникает ошибка типов в пакете dataloader.
Связана с несовпадением версий rxjs.

@atls/nestjs-dataloader
├── @nestjs/common@8.0.5
│   └── rxjs@7.5.4
└── rxjs@7.5.2

Пытался поменять версию rxjs на 7.5.2 для @nestjs/common@8.0.5 с использованием packageExtensions, но безуспешно. При установке зависимостей получаю

rxjs: This rule seems redundant when applied on the original package; the extension may have been applied upstream.

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

Note: This field is made to add dependencies; if you need to rewrite existing ones, prefer the resolutions field.


Сейчас пытаюсь что-то сделать через selective dependency resolutions


Добавил селективный resolution для пакета nestjs-dataloader

"resolutions": {
    "@atls/nestjs-dataloader/rxjs": "7.5.4",

Теперь разбираюсь с интеграционными тестами

@SlumberyDude
Copy link
Contributor

SlumberyDude commented Aug 10, 2023

Фикс интеграционных тестов

По закрытым PR видно, что интеграционные тесты падали с момента своего появления 20 февраля 2022 года https://github.com/atls/nestjs/pull/223/checks?check_run_id=5259703452 в тех же модулях


Гипотезы проблемы:

  • Что-то внутри @atls/code-test-worker, а именно внутри контента, который потом передается eval воркеру
  • typesense-typeorm: Возможно причина - библиотека testcontainers, которая позволяет запустить бд для теста внутри докер контейнера. Ее использует только данный модуль. Если быть точным, проблема внутри контейнера построенного по образу typesense/typesense:0.21.0
    Не хватало зависимости sqlite3, как и было написано в тексте ошибки.
  • kratos: Возможно не хватает зависимостей в тестовых модулях

@TorinAsakura
Copy link
Member

@SlumberyDude Это не отменяет наличия проблемы. Мне известно почему они падают, а тебе?

@TorinAsakura
Copy link
Member

@SlumberyDude А никто и не говорил про подмену. Я же тебе на созвоне объяснял, что это делается для добавления отсутствующих и в качестве примера привёл частую проблему с отсутствием типов в некоторых зависимостях…

@SlumberyDude
Copy link
Contributor

@TorinAsakura

Мне известно почему они падают, а тебе?

Пока что нет, сейчас копаю в сторону @atls/code-test-worker.

А никто и не говорил про подмену. Я же тебе на созвоне объяснял, что это делается для добавления отсутствующих и в качестве примера привёл частую проблему с отсутствием типов в некоторых зависимостях…

Тут я скорее исходил из твоего комментария к PR:

...это ровно тот случай, о котором я сегодня рассказывал, случай, когда необходимо использовать packageExtension, так как мы не можем позволить себе поставить самую последнюю версию rxjs из-за несовместимости окружения

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

@SlumberyDude
Copy link
Contributor

SlumberyDude commented Aug 10, 2023

На локале добавление sqlite3 в dev зависимости пакета typesense-typeorm решило проблему интеграционных тестов.

В kratos один и тот же тестовый кейс обрабатывается разным образом для unit и интеграционного теста. В интеграционном не может создать тестовый модуль и импортировать даже KratosModule, получаю ошибку разрешения зависимостей.

Nest can't resolve dependencies of the KratosExceptionFilter (?). Please make sure that the argument Object at index [0] is available in the KratosModule context.

    Potential solutions:
    - If Object is a provider, is it part of the current KratosModule?
    - If Object is exported from a separate @Module, is that module imported within KratosModule?
      @Module({
        imports: [ /* the Module containing Object */ ]
      })

Необходимо выяснить особенности запуска интеграционных тестов ярном.

@SlumberyDude
Copy link
Contributor

SlumberyDude commented Aug 11, 2023

Анализ интеграционных тестов в kratos

Цепочка инжектов зависимостей

KratosModule ---> KratosExceptionFilter --- ошибка тут  ---> KratosBrowserUrls ---> @Inject(KRATOS_MODULE_OPTIONS) private readonly options: KratosModuleOptions

Наличие последнего инжекта в данной цепочке вызывает ошибку.


Пофиксил.

Остался один нерабочий интеграционный тест на typesense-typeorm, но там что-то неясно, вчера же все проходило. На локале тоже возникала эта ошибка, прошла при повторном запуске.

@SlumberyDude
Copy link
Contributor

SlumberyDude commented Aug 14, 2023

Интеграционные тесты модуля typesense-typeorm

При чеке на гитхабе получаю ошибку

Error: expect(received).toBe(expected) // Object.is equality

Expected: 1
Received: 2
    at Object.<anonymous> (/home/runner/work/nestjs/nestjs/packages/typesense-typeorm/integration/typesense-typeorm.test.ts:72:30)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

Тест состоит из 2х кейсов, каждый из которых записывает объект в базу, затем ищет через typesense этот объект и проверяет, что нашелся один объект. То есть должен находить 1-1, тогда как в данном случае в ошибке выше видим, что нашлось 2 объекта во втором кейсе, т.е. результат 1-2

На локальном сервере тест проходит нестабильно.

Чаще всего оба кейса отрабатывают нормально, то есть результат 1-1 но встречаются различные комбинации, как
0-1, 1-2.


Имеется 2 проблемы

  1. После первого кейса база не очищается и казалось бы успешный случай 1-1 соответствует тому, что второй кейс находит объект записанный в 1 кейсе, а измененный объект в своем кейсе не видит (он не был изменен).
  2. При поиске объекта в БД с помощью typesense по каким-то причинам изменения в базе не успевают вступить в силу и поиск показывает неверный результат (не находит только что созданный/измененный объект). При этом данная ошибка носит вероятностный характер.

Предлагаемые способы исправления

    • Исправить проверку expect(result.found).toBe(1) во втором кейсе на expect(result.found).toBe(2) и не очищать базу между кейсами.
    • Очистить базу между кейсами
  1. Возможно выставление задержки между изменением в базе и поиском в typesense. В идеале разобраться, почему казалось бы синхронный код (изменение базы через await repository.save(entity)) не успевает внести изменения в базу к следующей инструкции.

Почему typesense не поспевает за обновлением в базе.

В модуле typesense-typeorm создается TypeOrmListenersBuilder, который создает слушателей. Последние отслеживают действия над репозиторием БД и вносят соответствующие обновления в typesense.

afterInsert(event: InsertEvent<any>) {
  this.mapper.insert(event.entity)
}

afterUpdate(event: UpdateEvent<any>) {
  this.mapper.update(event.entity)
}

Видим, что обновление typesense вызывается асинхронно после действий с БД не дожидаясь результата.

Поэтому тесты не проходят, typesense не успевает обновиться вслед за БД.

Считаю, что необходимо добавить дилей в тест между обновлением сущности в БД и поиском, чтобы typesense успевал синхронизировать изменения. 5 ms хватает

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants