Lodash upgrade and cleanup #2990

JoelMcKinnon · 2020-04-30T21:28:47Z

To resolve the security exception for lodash versioning and to clean up and remove many lodash methods, took the following steps:

Upgrade lodash and rename deprecated methods
Change web pack aliases for utils, add array-utils and tests
Change paths to use objectUtils alias
Convert ._extend, _.findIndex to native
Convert _.flatten, _.isEmpty, _.keyBy to use arrayUtils
Convert _.find, _.isArray, _.isEvery to native
Convert _.math.max, _.math.min, _.trim , _.invoke, _.isNaN to native
Add missing lodash imports

Closed related PR: #2970

Author Checklist

Changes address original issue? Yes
Unit tests included and/or updated with changes? Yes
Command line build passes? Yes
Changes have been smoke-tested? Yes

…-default-output

…s, updated imports

…debug

… lodash-debug

…odash-debug

akhenry

Looks really good. I don't think we can just bring "You don't need's" suggested code across verbatim though. It's covered by the MIT license. We would need to change them in some meaningful way, or just continue using lodash. I'm in favor of just continuing to use lodash in those cases for now, in the interests of getting this merged.

akhenry · 2020-05-13T20:57:45Z

src/api/utils/array-utils.js

+/* Retired lodash methods */
+
+// keyBy for array only
+export const keyBy = (array, key) => (array || []).reduce((acc, curr) => {


A note on this pattern of declaring functions. My preference here would be to just export named functions. We are assigning a new variable here unnecessarily, and anonymous functions can be harder to debug. I would also not be in favor of assigning a named function to a constant, so I think named functions are the way to go.

There are different schools of thought about this convention. If you're interested in reading more about the issue, read AirBnB's justification for using a similar convention for functions and the associated discussion. To me their recommended approach involves basically naming functions twice for almost no benefit (to avoid function hoisting). I actually think function hoisting is a good thing, but that's another discussion...

In this case the problem is resolved by removing the library array-utils entirely for the reasons mentioned elsewhere in this PR. If and when it's needed I'll refer back to this for guidance.

akhenry · 2020-05-13T22:48:48Z

.eslintrc.js

@@ -10,7 +10,9 @@ module.exports = {
    },
    "extends": [
        "eslint:recommended",
-        "plugin:vue/recommended"
+        "plugin:vue/recommended",
+        "plugin:lodash/recommended",


I don't think we need to keep the lodash linter enabled

removed along with the associated rules

akhenry · 2020-05-13T22:53:58Z

src/api/utils/array-utils.js

@@ -0,0 +1,46 @@
+/*****************************************************************************


I hate to say it, but I don't think we can in-house these functions. "you-don't-need" uses the MIT license, so if we use their code then we have to bring their license across as well. Which means we're no longer distributing under the Apache 2 license (approved by the NASA software release authority), we're also distributing under MIT.

OK - will remove array-utils.js, array-utilsSpec.js, and webpack alias, and re-implement the lodash methods for these.

akhenry · 2020-05-13T22:56:20Z

src/plugins/condition/ConditionManager.js

@@ -57,6 +57,7 @@ export default class ConditionManager extends EventEmitter {
            endpoint,
            this.telemetryReceived.bind(this, endpoint)
        );
+        // TODO check if this is needed


Remove commented code

akhenry · 2020-05-13T23:05:42Z

.eslintrc.js

-        "plugin:vue/recommended"
+        "plugin:vue/recommended",
+        "plugin:lodash/recommended",
+        "plugin:you-dont-need-lodash-underscore/compatible"


👍 I like this, to remind people to use native functions wherever possible.

akhenry · 2020-05-13T23:13:29Z

.eslintrc.js

@@ -22,6 +24,24 @@ module.exports = {
        }
    },
    "rules": {
+        "you-dont-need-lodash-underscore/uniq": "off",


Are all of these rules necessary? I don't see any usage of _.concat, or ._values for example? And we would definitely want to discourage usage of lodash in those cases.

I'll do a check and remove any unneeded rules

In PlotSeries.js and in a few other files there is a pattern like:

var newPoints = _(this.data) .concat(points) .sortBy(this.getXVal) .uniq(true, point => [this.getXVal(point), this.getYVal(point)].join()) .value();

Turning off the rules for _.uniq, _.concat, and _.values results in the following lint errors as a result of these patterns.

I assume each of these could be re-implemented natively, but not sure if you want me to tackle that in this PR.

same situation for _.map

OIC, I missed them because they're chained. Man, I would love to have the concat, values, and map checks enabled. It pretty significantly reduces the utility of having this check enabled.

Can we enable these checks globally, but disable them on a per-use basis? That way we can avoid future usage, but also not have to refactor these out right now.

That's a great idea - let me take a look at it.

…debug

…odash-debug

akhenry

Looking good. Still three minor issues.

akhenry · 2020-05-27T17:07:15Z

.eslintrc.js

@@ -10,7 +10,9 @@ module.exports = {
    },
    "extends": [
        "eslint:recommended",
-        "plugin:vue/recommended"
+        "plugin:vue/recommended",
+        // "plugin:lodash/recommended",


should be removed, not commented out.

akhenry · 2020-05-27T17:07:57Z

package.json

@@ -23,7 +23,9 @@
    "d3-time": "1.0.x",
    "d3-time-format": "2.1.x",
    "eslint": "5.2.0",
+    "eslint-plugin-lodash": "^6.0.0",


I'd like to remove this, I don't think we're going to get much value from it, and I'm keen to keep our dependency base as small as possible.

akhenry · 2020-05-27T17:10:04Z

package.json

    "markdown-toc": "^0.11.7",
    "marked": "^0.3.5",
    "mini-css-extract-plugin": "^0.4.1",
    "minimist": "^1.1.1",
-    "moment": "2.25.3",
+    "moment": "^2.25.3",


We need to use the exact version of moment here.

…ked version

…debug

…odash-debug

akhenry · 2020-05-27T17:41:06Z

@JoelMcKinnon Looks like there's a build issue now. It's due to a recently merged change that references testTools, which was renamed (correctly) testUtils in this branch. It's an easy fix.

shefalijoshi and others added 30 commits April 9, 2020 12:10

Ensures that results for a specific datapoint are evaluated atomically.

4e5cc84

Removes timestamp based evalutation from conditionManager

aa6c6cb

Remove unused code

66aa4f0

remove generating timestamp for telemetry data

80359e3

get results directly instead of using events

80c2504

remove unused listeners, events, and helpers

779a42c

linting

38b8f03

remove commented code

10679e5

telemetry criterion stores its own result

608d63a

refactor all/any telemetry criterion to use new evaluator

addeb63

tie in requests and eliminate unused code

7fb2bc9

use current timesystem to compare latest

717231f

scope function names

aaa60a1

AllTelemetryCriterion extends TelemetryCriterion

7e2cfa3

Merge branch 'master' into fix-default-output

9fa4707

Merge branch 'master' into fix-default-output

56e6fa6

Merge branch 'master' into fix-default-output

b2d8d64

fix telemetrycriterion and unit testing

80eab8b

Merge branch 'master' into fix-default-output

b9a85d9

fix unit tests

ba41c1a

Merge branch 'fix-default-output' of github.com:nasa/openmct into fix…

ae62b15

…-default-output

resolve merge conflicts

021d730

Merge branch 'master' of https://github.com/nasa/openmct

f9deb80

Merge branch 'master' of https://github.com/nasa/openmct

61d238a

Merge branch 'master' of https://github.com/nasa/openmct

0dff431

Merge branch 'master' of https://github.com/nasa/openmct

c185f77

upgraded lodash, renamed deprecated methods

1a8cb1e

added utils aliases to webpack, changed name of testTools to testUtil…

34b40ed

…s, updated imports

changed import paths to use objectUtils alias

397b603

convert ._extend, _.findIndex to native

38f199f

JoelMcKinnon requested a review from deeptailor April 30, 2020 21:28

JoelMcKinnon mentioned this pull request Apr 30, 2020

Lodash upgrade fix #2970

Closed

Joel McKinnon and others added 10 commits April 30, 2020 14:50

fix lint error

6108ab1

Merge branch 'master' into lodash-debug

0b35777

Merge branch 'master' into lodash-debug

05a49b0

Merge branch 'master' of https://github.com/nasa/openmct into lodash-…

042279f

…debug

Merge branch 'upgrade-moment' of https://github.com/nasa/openmct into…

3e8db0a

… lodash-debug

Merge branch 'lodash-debug' of https://github.com/nasa/openmct into l…

ed64612

…odash-debug

Merge branch 'master' into lodash-debug

d94cf81

Merge branch 'master' into lodash-debug

a48de29

unlock moment version

f86c046

Merge branch 'master' into lodash-debug

c294b66

akhenry requested changes May 13, 2020

View reviewed changes

Joel McKinnon and others added 8 commits May 13, 2020 18:00

removed array-utils and spec, re-implemented lodash as needed

c8e414d

removed reference to arrayUtils

d676f3b

Merge branch 'master' into lodash-debug

d61974c

added inline lint exclusions for specific lodash issues

a350dde

Merge branch 'master' of https://github.com/nasa/openmct into lodash-…

5de4774

…debug

Merge branch 'lodash-debug' of https://github.com/nasa/openmct into l…

701139b

…odash-debug

Merge branch 'master' into lodash-debug

0aa8d40

Merge branch 'master' into lodash-debug

535f125

akhenry requested changes May 27, 2020

View reviewed changes

Joel McKinnon added 3 commits May 27, 2020 10:34

removed commented code, lodash lint dependency, changed moment to loc…

6017892

…ked version

Merge branch 'master' of https://github.com/nasa/openmct into lodash-…

e9884da

…debug

Merge branch 'lodash-debug' of https://github.com/nasa/openmct into l…

4e70c7c

…odash-debug

renamed testTools to testUtils in spec

effad76

akhenry approved these changes May 27, 2020

View reviewed changes

akhenry merged commit 43628ad into master May 27, 2020

akhenry deleted the lodash-debug branch May 27, 2020 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lodash upgrade and cleanup #2990

Lodash upgrade and cleanup #2990

JoelMcKinnon commented Apr 30, 2020

akhenry left a comment

akhenry May 13, 2020

JoelMcKinnon May 14, 2020

akhenry May 13, 2020

JoelMcKinnon May 14, 2020

akhenry May 13, 2020

JoelMcKinnon May 14, 2020

JoelMcKinnon May 14, 2020

akhenry May 13, 2020 •

edited

Loading

JoelMcKinnon May 14, 2020

akhenry May 13, 2020

akhenry May 13, 2020

JoelMcKinnon May 13, 2020

JoelMcKinnon May 14, 2020

JoelMcKinnon May 14, 2020

akhenry May 18, 2020

JoelMcKinnon May 18, 2020

JoelMcKinnon May 18, 2020

akhenry left a comment

akhenry May 27, 2020

JoelMcKinnon May 27, 2020

akhenry May 27, 2020

JoelMcKinnon May 27, 2020

akhenry May 27, 2020

JoelMcKinnon May 27, 2020

akhenry commented May 27, 2020

		@@ -0,0 +1,46 @@
		/*****************************************************************************

Lodash upgrade and cleanup #2990

Lodash upgrade and cleanup #2990

Conversation

JoelMcKinnon commented Apr 30, 2020

akhenry 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

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

akhenry May 13, 2020 • 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

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

akhenry 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akhenry commented May 27, 2020

akhenry May 13, 2020 •

edited

Loading