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

deps(lodash): replace lodash per-method packages with full lodash #13695

Merged
merged 4 commits into from Mar 1, 2022
Merged

deps(lodash): replace lodash per-method packages with full lodash #13695

merged 4 commits into from Mar 1, 2022

Conversation

mhassan1
Copy link
Contributor

Summary
This PR replaces lodash per-method packages with imports from full lodash. This resolves an open CVE in lodash.set and removes the dependency on unmaintained lodash per-method packages. See #13694 for more information.

Related Issues/PRs
Resolves #13694

@mhassan1 mhassan1 requested a review from a team as a code owner February 23, 2022 05:41
@mhassan1 mhassan1 requested review from adamraine and removed request for a team February 23, 2022 05:41
@google-cla
Copy link

google-cla bot commented Feb 23, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@connorjclark
Copy link
Collaborator

We need to confirm bundle size (files in dist/ after yarn build-all) does not go up a lot from this change.

@@ -6,7 +6,7 @@
'use strict';

const path = require('path');
const isDeepEqual = require('lodash.isequal');
const { isEqual: isDeepEqual } = require('lodash');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect this to error because lodash is only commonjs.

How did you verify these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should error. I didn't do any verification, since I can't seem to run tests locally.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you using a supported Node version (>=14.15)? And I'd expect many tests to fail precisely because of this error. yarn build-all && yarn unit-cli should at least display useful messages when failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On master with node@14.15.1, yarn unit-cli fails with a bunch of these:

 FAIL  lighthouse-cli/test/cli/cli-flags-test.js
  ● CLI flags › settings are accepted from a file path

    expect(received).toMatchSnapshot()

    Snapshot name: `CLI flags settings are accepted from a file path 1`

    - Snapshot  - 1
    + Received  + 1

    @@ -1,7 +1,7 @@
      Object {
    -   "$0": "node_modules/jest/bin/jest.js",
    +   "$0": "node_modules/jest-worker/build/workers/processChild.js",
        "_": Array [
          "http://www.example.com",
        ],
        "budget-path": "path/to/my/budget-from-config.json",
        "budgetPath": "path/to/my/budget-from-config.json",

      33 |   }
      34 |
    > 35 |   expect(flags).toMatchSnapshot();
         |                 ^
      36 | }
      37 |
      38 | describe('CLI flags', function() {

      at snapshot (lighthouse-cli/test/cli/cli-flags-test.js:35:17)
      at Object.<anonymous> (lighthouse-cli/test/cli/cli-flags-test.js:76:5)

@mhassan1
Copy link
Contributor Author

@connorjclark This should be ready for re-test.

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

Successfully merging this pull request may close these issues.

Dependency lodash.set has an unpatched CVE
4 participants