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: remove unused npm packages #6229

Merged
merged 11 commits into from
Apr 28, 2020

Conversation

kieckhafer
Copy link
Member

@kieckhafer kieckhafer commented Apr 24, 2020

Many packages we used to need to install in the core api are no longer needed, as they are installed in each plugin instead. This PR removed the unneeded npm installations.

Removed packages:

  • @reactioncommerce/reaction-error
  • accounting-js
  • envalid
  • graphql
  • handlebars
  • later
  • ramda
  • simple-schema

Testing

  • See the app starts as expected
  • Running npm lint should tell you if any plugins are missing

Signed-off-by: Erik Kieckhafer <[email protected]>
@kieckhafer kieckhafer requested a review from aldeed April 24, 2020 22:50
@kieckhafer
Copy link
Member Author

kieckhafer commented Apr 24, 2020

@aldeed a few questions about other packages:

@kieckhafer kieckhafer changed the title [BLOCKED] chore: remove unused npm packages chore: remove unused npm packages Apr 25, 2020
Copy link
Contributor

@aldeed aldeed left a comment

Choose a reason for hiding this comment

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

@kieckhafer

  • I discovered the other day that graphql package is similar to react in that it can cause errors if there are two different versions installed. So I'm now thinking maybe graphql should be switched to be a peer dependency in the api-core package, rather then a regular dep. So if we do that, then graphql should remain installed in this repo.
  • lodash is used in jsdoc folder, but there is no point to building jsdoc docs anymore since this repo has very little code. So I suggest delete .reaction/jsdoc folder, search for and delete anything else related to jsdoc, and then also get rid of lodash.
  • I think you're correct that mocked package doesn't need to be listed, but not sure either. If you remove it and tests still pass, then I'd say we're good.

Also, it looks to me like faker, graphql-tools, and node-fetch can be moved to devDependencies because only test files import them, and sharp is unused and can be removed. Only semver and the @reactioncommerce packages are needed in dependencies

Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
@kieckhafer
Copy link
Member Author

kieckhafer commented Apr 25, 2020

@aldeed

Signed-off-by: Erik Kieckhafer <[email protected]>
Signed-off-by: Erik Kieckhafer <[email protected]>
@aldeed
Copy link
Contributor

aldeed commented Apr 26, 2020

@kieckhafer node_modules/sharp should continue to exist in the production image as long as any of the plugin packages depend on it. I'd say go ahead and remove it entirely, and after this is merged and we get a new Docker image build for trunk, we'll be able to run and verify that I'm correct. If I'm not, easy to add back before the next release.

aldeed
aldeed previously approved these changes Apr 28, 2020
Signed-off-by: Eric Dobbertin <[email protected]>
@aldeed
Copy link
Contributor

aldeed commented Apr 28, 2020

Removed the direct dependency on sharp. It didn't cause the package-lock to change so I think we're okay.

@aldeed aldeed merged commit b153a47 into trunk Apr 28, 2020
@aldeed aldeed deleted the chore-kieckhafer-removeUnusedNPMPackages branch April 28, 2020 20:21
@kieckhafer kieckhafer mentioned this pull request Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants