-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Node #1818
Node #1818
Conversation
Added workflow for video crop
@Arri98
|
This node update also updates npm:
which uses a new [1] https://docs.npmjs.com/cli/v8/configuring-npm/package-lock-json#lockfileversion Currently, the file is converted on every build and test execution:
We should do the conversion once, and commit the format change as part of this PR. This will also reduce the noise on future dependency update PRs. The file format can be converted using:
for all JS projects:
@Arri98 Could you update the PR with these changes, please? |
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.
@Arri98 Looking good :-) Just a couple of nits, and the package-lock.json
update (I think that it makes sense to address the file format change in this PR).
@@ -26,7 +26,7 @@ | |||
"karma-mocha-reporter": "^2.2.5", | |||
"karma-sinon": "^1.0.5", | |||
"mocha": "^9.0.3", | |||
"mock-require": "^1.3.0", | |||
"mock-require": "^3.0.3", |
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.
We should also update package-lock.json
, which can be done by running:
npm install --package-lock-only --ignore-scripts --no-audit --no-fund
(This will also update the file format, as mentioned in my previous comment).
@@ -1,7 +1,6 @@ | |||
/* global require, setInterval, clearInterval, exports */ | |||
|
|||
/* eslint-disable no-param-reassign */ | |||
|
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.
Nit: Can we remove this whitespace-only change? (It adds some noise to the PR).
@@ -157,6 +156,7 @@ if (global.config.erizoController.listen_ssl) { | |||
} | |||
|
|||
server.listen(global.config.erizoController.listen_port); | |||
|
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.
Nit: Can we remove this whitespace-only change? (It adds some noise to the PR).
@Arri98 @lodoyun We have been testing this change (applied to We have performed a load test using a single stream with upto 1200 subscribers: The load tests ran reliably. Also the CPU utilization of the ErizoJS of this PR (test_id=13) is very similar to the pre-v10.13 one (test_id=12): The latter was expected, as the node version upgrade should only affect the control plane (the data plane is handled by the C++ implementation). We have also been dogfooding the PR change using the new version in our internal meetings (also without a glitch). |
Updated lockfiles and removed the whitespaces in the erizo_controlle file |
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.
This looks good to me. Thanks! 👍
Upated Node to latest LTS. Made minor changes to the ErizoAPI and updated mock-require because compatibility issues.
[] It needs and includes Unit Tests
Changes in Client or Server public APIs
[] It includes documentation for these changes in
/doc
.