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

chore: Clean up deps and npm audit issues #2058

Merged
merged 20 commits into from
Oct 16, 2024

Conversation

jdeichert
Copy link
Contributor

@jdeichert jdeichert commented Oct 3, 2024

Motivations

This PR cleans up a number of High Severity audit issues (from 19, down to 1). To achieve this, I tried to make the minimum amount of changes required. Along the way, I attempted to clean up some old, unnecessary dependencies as well. Check out my inline comments for more context.

This also closes a number of open Dependabot PRs:

Before After
Screenshot 2024-10-08 at 11 39 35 AM Screenshot 2024-10-08 at 11 27 31 AM

Changes

Changed

  • Updated plop from v2 to v4
  • Updated all storybook deps to match 7.6.20
  • Updated react-native to match JM's version
    • Fixed some type-related issues due to this

Removed

  • Unused dependencies

Security

  • Reduced 19 high sev issues down to 1

Testing

To test this:

  • install the pre-release versions (comment) inside JO/JF/JM
  • poke around and make sure everything seems to look normal
    • I ran through creating client->request->quote->job->invoice as a basic flow to ensure things are working nicely
  • I also ran through every storybook story to make sure they loaded fine. I didn't notice any problems there.
  • Test out plop v4 via npm run generate to ensure it still works

Changes can be tested via Pre-release

These haven't been used directly by the design package for awhile.. probably since the big design token rewrite.

Also removed from the root package.json as it isn't a direct dependency.. storybook uses it as a sub dep.
@jdeichert jdeichert changed the title CLEANUP Clean up deps and npm audit issues part 2 chore: CLEANUP Clean up deps and npm audit issues part 2 Oct 3, 2024
@jdeichert jdeichert changed the title chore: CLEANUP Clean up deps and npm audit issues part 2 chore: Clean up deps and npm audit issues part 2 Oct 3, 2024
Copy link

cloudflare-workers-and-pages bot commented Oct 3, 2024

Deploying atlantis with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6d2c97c
Status: ✅  Deploy successful!
Preview URL: https://330880bc.atlantis.pages.dev
Branch Preview URL: https://cleanup-bumps-deps-attempt2.atlantis.pages.dev

View logs

@jdeichert jdeichert force-pushed the CLEANUP/bumps-deps-attempt2 branch from 854958c to a8820f2 Compare October 3, 2024 18:28
width: string | number;
height: string | number;
width: number;
height: number;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After upgrading react-native, it was complaining about these prop types on our Icon component. Removing string fixed it.

Notably our components-native package uses the version of react-native inside JM (it's a peer dep) which is already on 0.73.9. This means this should have zero impact as we haven't had any reported issues with icons yet.

@jdeichert jdeichert force-pushed the CLEANUP/bumps-deps-attempt2 branch from 5fea461 to 7bc23ad Compare October 3, 2024 19:11
"main": "src/index.ts",
"scripts": {},
"devDependencies": {
"react": "^16.8.6"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't necessary thankfully 😅

"react-intl": "^6.4.2",
"react-native": "^0.71.7",
"react-native": "0.73.9",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upgraded to match JM's version. Not using the caret as I wanted to keep it in sync with JM instead of auto bumping patch versions.

@@ -134,9 +132,8 @@
"react": "^18.2.0",
"react-dom": "18.2.0",
"react-ga4": "^2.1.0",
"react-hook-form": "^7.30.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unnecessary at the root level, it's a dep in both components and components-native

@@ -125,7 +124,6 @@
"lodash": "^4.17.21",
"metro-react-native-babel-preset": "^0.76.0",
"plop": "^2.5.3",
"postcss": "^8.4.40",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed as it's not directly used... Storybook imports it as a dep.

@@ -69,26 +69,25 @@
"@jobber/generators": "*",
"@jobber/stylelint-config": "*",
"@react-native-community/datetimepicker": "^7.1.0",
"@storybook/addon-actions": "7.6.11",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upgraded more storybook-related deps to match the recently merged version bump.

"@types/react-native-uuid": "^1.4.0",
"metro-react-native-babel-preset": "^0.76.0",
"react-test-renderer": "^18.2.0",
"typescript": "^4.9.5"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typescript is shared at the root now, removed from all individual packages.

@@ -400,7 +400,6 @@ function InputTextInternal(
styleOverride?.inputText,
loading && loadingType === "glimmer" && { color: "transparent" },
]}
// @ts-expect-error - does exist on 0.71 and up https://github.com/facebook/react-native/pull/39281
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer necessary as we bumped react-native to match JM

Comment on lines -451 to -454
"@testing-library/jest-dom": "^5.16.5",
"@testing-library/react": "^14.0.0",
"@testing-library/react-hooks": "^7.0.2",
"@testing-library/user-event": "^14.5.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shared at the root package.json

Comment on lines -70 to -73
"postcss": "^8.4.40",
"postcss-calc": "^10.0.0",
"postcss-cli": "^11.0.0",
"postcss-import": "^16.1.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These haven't been used directly by the design package for awhile.. probably since the big design token rewrite.

@jdeichert jdeichert force-pushed the CLEANUP/bumps-deps-attempt2 branch from 8dbb845 to b3285a4 Compare October 3, 2024 19:24
`npm i --legacy-peer-deps=true` and then another `npm i`
npm update express
npm update path-to-regexp
npm update @nrwl/devkit
npm update npm-registry-fetch
npm update make-fetch-happen
npm update socks-proxy-agent
npm update socks
@@ -28,7 +28,6 @@
"lint:js": "eslint --ext .js,.jsx,.ts,.tsx . --ignore-pattern node_modules/ --ignore-pattern 'packages/design/src/assets/**'",
"lint:markdown": "prettier --check '**/*.{md,mdx}' --write",
"pack": "npm run bootstrap && npm pack -w",
"plop": "echo '🚧 `npm run plop` is now `npm run generate`'",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upgraded plop to v4. This ^ warning has probably been around long enough, so I removed it. I can add it back if anyone depends on it 👀

@@ -124,19 +121,16 @@
"lint-staged": "^9.4.2",
"lodash": "^4.17.21",
"metro-react-native-babel-preset": "^0.76.0",
"plop": "^2.5.3",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upgraded to v4 (a few lines below)

"postcss-import": "^12.0.1",
"postcss-loader": "^4.2.0",
"postcss-preset-env": "^8.3.0",
"prettier": "^2.4.1",
"prop-types": "^15.7.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unnecessary dep

Comment on lines +180 to +185
"lerna": {
"semver": "^5.7.2 || ^6.3.1 || ^7.5.2",
"make-dir": {
"semver": "^6.3.1"
}
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

semver is one of our high severity issues (see npm audit on master). I upgraded it within a bunch of subdeps, but lerna was difficult and depends on an exact version (they are not using ^), so the only way to override it is like this.

I'll publish a pre-release to verify lerna is still working 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a comment, we may want to make a ticket for updating lerna / remove it if we want to transition later but that is a not in this PR thing

@jdeichert jdeichert changed the title chore: Clean up deps and npm audit issues part 2 chore: Clean up deps and npm audit issues Oct 4, 2024
Copy link

github-actions bot commented Oct 8, 2024

Published Pre-release for 6d2c97c with versions:

  - @jobber/[email protected]+6d2c97c7
  - @jobber/[email protected]+6d2c97c7
  - @jobber/[email protected]+6d2c97c7

To install the new version(s) for Web run:

npm install @jobber/[email protected]+6d2c97c7 @jobber/[email protected]+6d2c97c7

To install the new version(s) for Mobile run:

npm install @jobber/[email protected]+6d2c97c7 @jobber/[email protected]+6d2c97c7

@jdeichert jdeichert marked this pull request as ready for review October 9, 2024 20:33
@jdeichert jdeichert requested a review from a team as a code owner October 9, 2024 20:33
@@ -109,7 +107,6 @@
"eslint": "^8.36.0",
"eslint-formatter-codeframe": "^7.32.1",
"eslint-import-resolver-alias": "^1.1.2",
"eslint-plugin-jest": "^22.6.4",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used inside packages/eslint-config/package.json

@Aiden-Brine
Copy link
Contributor

Aiden-Brine commented Oct 10, 2024

Seeing a bug on the home page when I use the pre-release on Jobber Frontend. I removed the pre-release and re-added it so I am confident the bug is related to the changes.
Screenshot 2024-10-10 at 2 59 51 PM

@jdeichert
Copy link
Contributor Author

@Aiden-Brine I'm not seeing that 🤔 I tested both light and dark mode.

Screenshot 2024-10-10 at 3 29 52 PM Screenshot 2024-10-10 at 3 29 49 PM

Are you clearing your vite cache between rebuilds by chance?

@Aiden-Brine Aiden-Brine self-requested a review October 11, 2024 20:27
@MichaelParadis MichaelParadis self-requested a review October 15, 2024 15:18
@@ -448,10 +448,6 @@
"@rollup/plugin-commonjs": "^25.0.7",
"@rollup/plugin-node-resolve": "15.2.3",
"@rollup/plugin-typescript": "^11.1.6",
"@testing-library/jest-dom": "^5.16.5",
Copy link
Contributor

Choose a reason for hiding this comment

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

Changes from the actual bundled output of components is just file renames with different hashes and updates to imports to go the the renamed files

@@ -111,8 +111,8 @@ function buildSVGStyle(
const iconSizeStyle = iconSizes.tokens[size];
const svgStyle: {
fill?: string;
width: string | number;
height: string | number;
width: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Checking the compiled output for design it is the exact same except for this but there are no typescript errors caused by this change

@@ -400,7 +400,6 @@ function InputTextInternal(
styleOverride?.inputText,
loading && loadingType === "glimmer" && { color: "transparent" },
]}
// @ts-expect-error - does exist on 0.71 and up https://github.com/facebook/react-native/pull/39281
readOnly={readonly}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only diff in the bundle output but there are no typescript errors caused by this

@MichaelParadis
Copy link
Contributor

@Aiden-Brine I was unable to see this. My assumption is that some file is being caused try cleaning all of node modules and trying a new install.

Copy link
Contributor

@MichaelParadis MichaelParadis left a comment

Choose a reason for hiding this comment

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

LGTM!

@MichaelParadis MichaelParadis merged commit fd3222f into master Oct 16, 2024
13 checks passed
@MichaelParadis MichaelParadis deleted the CLEANUP/bumps-deps-attempt2 branch October 16, 2024 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants