-
Notifications
You must be signed in to change notification settings - Fork 44
Fix/esbuild #179
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
Fix/esbuild #179
Conversation
const RELATIVE_PATH = process.env.TEST_ENVIRONMENT ? '../' : './' | ||
const RESOLVED_PATH = path.join(__dirname, RELATIVE_PATH, CERTIFICATES_FOLDER) | ||
const FALLBACK_INTERNAL_ROOT_CERTS = path.join(RESOLVED_PATH, 'internal.pem'); | ||
const FALLBACK_SYSTEM_ROOT_CERTS = path.join(RESOLVED_PATH, 'system.pem'); |
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.
Почему бы содержимое сертификатов не положить в текстовом виде в переменную в какой-нибудь js модуль и не реквайрить их как обычно? Кажется это решит все проблемы (в том числе с бандлерами).
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.
// certs.js
module.exports = {
internal: '<text_contenxt_of_cert>',
fallback: '<text_contenxt_of_cert>'
}
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.
Я вполне с этим согласен -- это предлагалось, однако по уму их надо бандлить вебпком во время компиляции из файлов\по урлу. Если есть консенсус по этому вопросу могу реализовать как через бандлер, так и переменной в .js
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.
А расскажите, в какой ситуации здесь появляются бандлеры? Ведь ydb-nodejs-sdk - это нодовая библиотека и, кажется, нет особого смысла бандлить/минифицировать код, который выполняется на ноде и не доезжает до клиента.
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.
Смысл еще как есть. Кроме того, что код собирается в один файлик (что при прочих равных удобно) так еще и размер артефакта значительно меньше (учитывая что все node_modules не надо тащить). В контексте serverless функции очень значимый момент.
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.
Хотелось бы понять можно ли всё-таки добавить webpack\esbuild для бандлинга cert или вставить сертификаты руками в код как предлагал @DavyJohnes
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.
Я правильно понимаю, что бандлер и зависимости ydb-sdk тоже в бандл положит?
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.
Да, он соберёт всё в один файл
Я тут посмотрел на размер system.pem, он довольно здоровый, поэтому я бы рекомендовал рассмотреть возможность скачивания его с публичного хранилища
const RELATIVE_PATH = process.env.TEST_ENVIRONMENT ? '../' : './' | ||
const RESOLVED_PATH = path.join(__dirname, RELATIVE_PATH, CERTIFICATES_FOLDER) | ||
const FALLBACK_INTERNAL_ROOT_CERTS = path.join(RESOLVED_PATH, 'internal.pem'); | ||
const FALLBACK_SYSTEM_ROOT_CERTS = path.join(RESOLVED_PATH, 'system.pem'); |
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.
А расскажите, в какой ситуации здесь появляются бандлеры? Ведь ydb-nodejs-sdk - это нодовая библиотека и, кажется, нет особого смысла бандлить/минифицировать код, который выполняется на ноде и не доезжает до клиента.
7a5aeae
to
fb8b835
Compare
fb8b835
to
eba62c9
Compare
This is a poor man's fix for bundling compilers like esbuild\webpack that require paths to be relative to the entry point. Certs folder is moved into cjs\esm dists so we can utilize universal path './certs' that would work for default tsc compilation and would allow copying the certs folder to whatever path bundler is gonna output compiled entry point. Some sensible error communication is also added This commit is not marked as breaking because if certs folder had not been found when grpcs is present in previous implementation the connection would not succeed and\or "File not found" error threw
eba62c9
to
1834ced
Compare
3033e72
to
c3f6b88
Compare
throw new Error(certificateNotFoundMessage) | ||
} | ||
|
||
const internalRootCertificates = fs.readFileSync(FALLBACK_INTERNAL_ROOT_CERTS) |
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.
@esseswann @tsufiev а почему решили оставить чтение с fs? Это тоже потребует дополнительной настройки бандлера. Может таки положим в виде строки в js модуль?
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.
Я не против, но меня немного смущает размер system.pem, возможно стоит обдумать как это можно сделать конфигурируемо
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.
Так а какая разница? Он тут так или иначе в памяти окажется, читаешь ты его с fs или реквайришь как js модуль.
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.
Там есть кейс при котором его чтение необязательно, следовательно мы просто разжираем размер бандла. Это правда не меняет ситуацию относительно текущей, но теоретически можно было бы этого избежать
Resolves #173
Package version fix is relatively straightforward
For certificates search path refer to the ced8e20
It is still recommended to use some kind of bundler like esbuild itself to bake the certs and version into the compiled code