search

Quansight / Quansight-website

đź’» Source code for Quansight Labs website
https://labs.quansight.org
21 stars 50 forks source link

Audit and prune dependencies #444

Open gabalafou opened 2 years ago

gabalafou commented 2 years ago

Today I was reviewing pull requests from dependabot, and I'm not sure that we're actually using the dependencies that the bot is updating. This is especially true of the dev dependencies, but I think there may even be some normal dependencies that are not used by the quansight app.

The entire package.json could use an audit and review.

gabalafou commented 2 years ago

Some concrete tasks:

bskinn commented 2 years ago

486 already open for removing sass

Surafel-Getachew commented 1 year ago

Here are some of the dependencies that we are not using

and there are a lot of devDependencies that aren’t being used

But there is high chance that it might be a false positive as there might be somewhere deep that we’re using this dependencies so it needs time to confirm that also checked dependabot and I think it’s set up to only update the dependencies and also checked the PR by dependabot and it’s only bumping version’s.

bskinn commented 1 year ago

@gabalafou, I think we need to define a standard test sequence to ~confidently tell us when a dependency is actually used or not.

The most thorough approach I can think of involves working in a test branch and, for a single candidate dependency X, doing something like:

  1. Prep work a. Remove X from package.json b. Delete node_modules and package-lock.json c. Run npm install
  2. Run each of the npm run subcommands and ensure they complete successfully and behave as expected (each site loads ok for the start:<site> commands, etc.)
    • We might discover that some of these subcommands don't work or that we don't need them?
  3. Check the Vercel builds a. Commit the changes to package.json and package-lock.json b. Push to GitHub c. Check the builds

Thoughts?

Surafel-Getachew commented 1 year ago

Here are some of the dependencies that we are not using

  • core-js
  • regenerator-runtime
  • slick-carousel
  • swr
  • tslib

and there are a lot of devDependencies that aren’t being used

  • @graphql-codegen/add
  • @graphql-codegen/import-types-preset
  • @graphql-codegen/introspection
  • @graphql-codegen/near-operation-file-preset
  • @graphql-codegen/typescript
  • @graphql-codegen/typescript-document-nodes
  • @graphql-codegen/typescript-operations
  • @graphql-codegen/typescript-react-apollo
  • @nrwl/cypress
  • @nrwl/linter
  • @nrwl/tao
  • @nrwl/web
  • @nrwl/workspace
  • @testing-library/react-hooks
  • autoprefixer
  • codegen
  • eslint-plugin-prettier
  • eslint-plugin-react
  • eslint-plugin-react-hooks
  • minimist
  • nx-stylelint
  • react-test-renderer

But there is high chance that it might be a false positive as there might be somewhere deep that we’re using this dependencies so it needs time to confirm that also checked dependabot and I think it’s set up to only update the dependencies and also checked the PR by dependabot and it’s only bumping version’s.

I've also checked using this npm library it tries to identify unused dependencies but it also have short comings like it list some dependencies as unused when they are not in the syntax files, but they are used by other dependencies and configuration files, scripts etc..

bskinn commented 1 year ago

Yeah, I believe that many of the devDependencies don't show in the codebase - I expect many of them are used by the npm run dev workflow commands.

Surafel-Getachew commented 1 year ago

I agree with the approach here, but little modification I'd suggest doing for all the candidates in the unused dependencies list (not dev dependencies) because it's a short list now 5 but after some deep check it might even get shorter so I will put the list of dependencies that are removed during the testing process and the results.

what do you think? @bskinn

bskinn commented 1 year ago

but after some deep check it might even get shorter

By this, do you mean that you might discover that one or more of these five packages actually are used in the project, and thus you can eliminate them from consideration for removal, without @gabalafou's or my input? If so, then yes, sounds good.

More generally, I agree that you should focus on the dependencies first. But, I think we do want to also evaluate the devDependencies -- unless @gabalafou thinks the devDependencies aren't worth the time.

Surafel-Getachew commented 1 year ago

but after some deep check it might even get shorter

By this, do you mean that you might discover that one or more of these five packages actually are used in the project, and thus you can eliminate them from consideration for removal, without @gabalafou's or my input? If so, then yes, sounds good.

More generally, I agree that you should focus on the dependencies first. But, I think we do want to also evaluate the devDependencies -- unless @gabalafou thinks the devDependencies aren't worth the time.

Yes that's exactly what I meant.

bskinn commented 1 year ago

Yes that's exactly what I meant.

Great -- yes, please proceed!

bskinn commented 1 year ago

Just came across depcheck, might be helpful: https://www.npmjs.com/package/depcheck