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

WIP: Resolve npm vulnerabilities (part 3 of 3) #4977

Closed
wants to merge 2 commits into from
Closed

Conversation

@bsclifton
Copy link
Member

bsclifton commented Mar 18, 2020

Fixes brave/brave-browser#8722

Submitter Checklist:

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.
@bsclifton bsclifton added this to the 1.8.x - Nightly milestone Mar 18, 2020
@bsclifton bsclifton self-assigned this Mar 18, 2020
@bsclifton
Copy link
Member Author

bsclifton commented Mar 18, 2020

While this PR gets rid of dependency errors, the storybook commands don't seem to work properly with optional-dev-dependency ☹️

@bsclifton bsclifton force-pushed the bsc-fix-npm branch from 0ac4484 to 7bf2fdc Mar 18, 2020
@bsclifton
Copy link
Member Author

bsclifton commented Mar 18, 2020

Huge props to @ryanml for recommending https://www.npmjs.com/package/npm-force-resolutions 😻

@bsclifton bsclifton force-pushed the bsc-fix-npm branch from 7bf2fdc to e0424d6 Mar 18, 2020
@bsclifton bsclifton requested review from cezaraugusto, petemill and ryanml Mar 18, 2020
@bsclifton
Copy link
Member Author

bsclifton commented Mar 18, 2020

Almost ready for review - need to troubleshoot the travis-ci problem with npm run build-storybook

I believe @cezaraugusto is looking at this

@@ -307,6 +308,7 @@
"less-loader": "^4.0.5",
"mkdirp": "^0.5.1",
"mz": "^2.7.0",
"npm-force-resolutions": "0.0.3",

This comment has been minimized.

Copy link
@petemill

petemill Mar 18, 2020

Member

This shouldn't be necessary. The npx command runs a binary in an npm repo (in this case the npm-force-resolutions). It should download the repo and run the necessary command on-demand. Brave itself doesn't need to have the dependency in its source tree as far as I can tell.

This comment has been minimized.

Copy link
@bsclifton

bsclifton Mar 18, 2020

Author Member

right - but preinstall is running npx npm-force-resolutions. If we don't add this, nothing would happen? 🤔

@bsclifton
Copy link
Member Author

bsclifton commented Mar 19, 2020

Closing - this approach caused problems when using with brave-core-crx-packager ☹️

@bsclifton bsclifton closed this Mar 19, 2020
@bsclifton bsclifton deleted the bsc-fix-npm branch Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.