[In Progress] Chore: TR-3008 update 3rd party deps #175

Nicholass · 2023-08-31T10:31:55Z

% npm audit
found 0 vulnerabilities

Replaced deprecated lodash method calls

Tested with:

npm run test:dev
npm run build

oatymart

The current changes look too risky, but don't give up. Run a full TAO instance to understand which libs could or couldn't be safely upgraded.

oatymart · 2023-08-31T12:07:11Z

package.json

+        "handlebars": "^4.7.8",
        "interactjs": "1.3.4",
-        "jquery": "1.9.1",
+        "jquery": "^3.7.1",
        "jquery-mockjax": "^2.6.0",
        "jquery-simulate": "^1.0.2",
-        "lodash": "2.4.1",
+        "lodash": "^4.17.21",


I'm not sure we can get away with casually bumping all these libs.

The reason is that tao-core-sdk is not used in isolation, it is used as part of a fully-installed TAO where several other components are still using handlebars 1.3.0, jquery 1.9.1 and lodash 2.4.1. In some cases we would have a conflict in the browser (jquery for sure / lodash not sure).

That's why we intend to handle the upgrade of these libs with a dedicated task designed to upgrade each one in all places at the same time.

Handlebars: https://oat-sa.atlassian.net/browse/TR-3264

Jquery: https://oat-sa.atlassian.net/browse/TR-3262

Lodash: task doesn't exist, but perhaps it should

The overall initiative to move some lodash usages to the v4 API is good, though. If you want to concentrate on that, you can continue, but be sure to test this library installed in tao-core in a full TAO Community.

I realized that this one is indeed to risky. I guess I'll go with npm audit fix without --force for now.

BTW, as Martin explained, you should have a look at how the FE packages relate to each other on TAO.

We have a bunch of libraries that are dependent on each other, hierarchically. When updating one, you need to spread the change across all others.

As a quick overview, we have in order:

tao-core-shared-libs

tao-core-libs

tao-core-sdk

tao-core-ui

tao-item-runner

tao-item-runner-qti

tao-test-runner

tao-test-runner-qti

And besides that, a galaxy of smaller packages that are consumed for the tooling, like:

grunt-tao-bundle

browserslist-config-tao

eslint-config-tao

prettier-config

etc.

And some other specific libraries...

In TAO, we also have extensions that will contain frontend code too, consuming and relying on the above-mentioned libs.

A change in lodash, will impact all of that. The same with jQuery, Handlebars, Select2... Because these are peer dependencies, and only one version at a time is supported. If you update one in a package, it will conflict with the version required in another package.

In other words, we cannot update a single package without taking care of all of the others that depend on it

oatymart · 2023-08-31T12:07:15Z

package.json

@@ -81,7 +81,7 @@
        "rollup-plugin-istanbul": "^2.0.1",
        "rollup-plugin-json": "^4.0.0",
        "rollup-plugin-node-resolve": "^5.2.0",
-        "select2": "3.5.1",
+        "select2": "^4.1.0-rc.0",


Have you checked what breaking changes are announced in select2 v4? Could it affect us?

issue: This breaks TAO. We cannot upgrade Select2 for the time being, the code is not compatible, and a significant migration is required. Moreover, we should not use RC without a very good reason.

oatymart · 2023-08-31T12:09:27Z

src/core/databinder.js

@@ -584,7 +584,7 @@ DataBinder.prototype._setNodeValue = function _setNodeValue($node, value) {
        }

        //assign value
-        if (_.contains(['INPUT', 'SELECT', 'TEXTAREA'], $node[0].nodeName)) {
+        if (_.includes(['INPUT', 'SELECT', 'TEXTAREA'], $node[0].nodeName)) {


A better replacement might be vanilla Array.includes(). It would ease the work of upgrading lodash v2 to v4, if we end up doing that later.

oatymart · 2023-08-31T12:09:38Z

.github/workflows/continuous-integration.yml

@@ -25,7 +25,7 @@ jobs:
                  node-version: 18.x
                  registry-url: https://registry.npmjs.org
            - name: Install packages
-              run: npm ci
+              run: npm ci --legacy-peer-deps


This needs explanation too.

jsconan

Well, I understand the good intent, but I'm afraid this PR needs to be closed. None of the proposed version upgrades is acceptable in the state.

FYI, we have specific tickets for taking care of these major upgrades.

jsconan · 2023-09-01T07:41:43Z

.github/workflows/continuous-integration.yml

@@ -25,7 +25,7 @@ jobs:
                  node-version: 18.x
                  registry-url: https://registry.npmjs.org
            - name: Install packages
-              run: npm ci
+              run: npm ci --legacy-peer-deps


issue: This is adding an exception in the install flow. Setting install options in the CI won't remove issues when installing locally. A better approach is to rely on a local .npmrc so that the experience is seamless for everyone.

jsconan · 2023-09-01T07:44:49Z

package.json

+        "handlebars": "^4.7.8",
        "interactjs": "1.3.4",
-        "jquery": "1.9.1",
+        "jquery": "^3.7.1",


issue: This is breaking everything in TAO. Please do not apply blindly a version bump without properly migrating the code, especially when breaking changes are involved.

FYI, we already have tasks for taking care of these particular dependencies, as Martin already spotted.

jsconan · 2023-09-01T07:47:00Z

package.json

@@ -81,7 +81,7 @@
        "rollup-plugin-istanbul": "^2.0.1",
        "rollup-plugin-json": "^4.0.0",
        "rollup-plugin-node-resolve": "^5.2.0",
-        "select2": "3.5.1",
+        "select2": "^4.1.0-rc.0",


issue: This breaks TAO. We cannot upgrade Select2 for the time being, the code is not compatible, and a significant migration is required. Moreover, we should not use RC without a very good reason.

reverted `--force` part. tests now good

github-actions · 2023-09-05T13:38:37Z

Version

Target Version	2.0.3
Last version	2.0.2

There are 0 BREAKING CHANGE, 0 feature, 0 fix

github-actions · 2023-09-05T13:39:50Z

Coverage Report

Totals
Statements:	90.79% ( 2592 / 2855 )
Methods:	95.19% ( 594 / 624 )

Nicholass · 2023-10-06T07:54:41Z

it looks like less "Aggresive" version bump gives nothing but 1 high issue in addition
with the fix: 5 vulnerabilities (2 moderate, 1 high, 2 critical)
current dev: 6 vulnerabilities (4 moderate, 2 critical)
In dev the vulnerable packages are:
select2, jquery, uglify-js so I guess I'd close this PR as redundant

Nicholass requested review from jsconan, victordp86, wazelin and oatymart August 31, 2023 10:32

Nicholass added the dependencies Pull requests that update a dependency file label Aug 31, 2023

Nicholass changed the title ~~Chore: TR-3008 update 3rd party deps~~ [In Progress] Chore: TR-3008 update 3rd party deps Aug 31, 2023

oatymart requested changes Aug 31, 2023

View reviewed changes

jsconan requested changes Sep 1, 2023

View reviewed changes

chore: tr-3008 update 3rd party dependencies

a56c89c

reverted `--force` part. tests now good

Nicholass force-pushed the chore/tr-3008/update-3rd-party-deps branch from eed61c5 to a56c89c Compare September 5, 2023 13:38

wazelin requested a review from taopkorczak September 6, 2023 14:32

Nicholass closed this Oct 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[In Progress] Chore: TR-3008 update 3rd party deps #175

[In Progress] Chore: TR-3008 update 3rd party deps #175

Nicholass commented Aug 31, 2023

oatymart left a comment

oatymart Aug 31, 2023

Nicholass Aug 31, 2023

jsconan Sep 1, 2023 •

edited

Loading

oatymart Aug 31, 2023

jsconan Sep 1, 2023

oatymart Aug 31, 2023

oatymart Aug 31, 2023

jsconan left a comment

jsconan Sep 1, 2023

jsconan Sep 1, 2023

jsconan Sep 1, 2023

github-actions bot commented Sep 5, 2023

github-actions bot commented Sep 5, 2023

Nicholass commented Oct 6, 2023

[In Progress] Chore: TR-3008 update 3rd party deps #175

[In Progress] Chore: TR-3008 update 3rd party deps #175

Conversation

Nicholass commented Aug 31, 2023

oatymart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsconan Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsconan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 5, 2023

Version

github-actions bot commented Sep 5, 2023

Coverage Report

Nicholass commented Oct 6, 2023

jsconan Sep 1, 2023 •

edited

Loading