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

build: FORMS-1679 remove unused components dependencies #1554

Merged

Conversation

WalterMoar
Copy link
Collaborator

@WalterMoar WalterMoar commented Dec 23, 2024

It’s a best practice to have as few dependencies as possible. The “components” project that houses the form.io components has many packages in its dependencies that don’t appear to be used. Remove whatever is not needed.

Note: while we can search the code for package use, it’s entirely possible that some packages are needed but not in an obvious way. After this change the app should be monitored for errors.

Acceptance Criteria

  • Unused dependencies are removed
  • The app continues to function as before

Description

Type of Change

build (change in build system or dependencies)

Checklist

  • I have read the CONTRIBUTING doc
  • I have checked that unit tests pass locally with my changes
  • I have run the npm script lint on the frontend and backend
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • I have approval from the product owner for the contribution in this pull request

Further comments

Including the dev dependencies this reduces the package count from 656 to 461. A side effect is that it will probably get rid of some security vulnerabilities picked up by dependabot.

This comment has been minimized.

Comment on lines -25 to -26
"test:coverage": "nyc --reporter=text mocha --reporter spec './{,!(node_modules)/**/}*.spec.js'",
"test": "mocha --require ts-node/register --reporter spec './{,!(node_modules)/**/}*.spec.ts'",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since we don't have tests for the components, removing these scripts allows us to further reduce the number of dependencies.

components/package.json Outdated Show resolved Hide resolved
We don't have tests for these components, so also removing the tests script to remove even more dependencies.
@WalterMoar WalterMoar force-pushed the build/1679-remove-components-dependencies branch from f084dc9 to a30a024 Compare December 24, 2024 17:49
Copy link
Collaborator

@jasonchung1871 jasonchung1871 left a comment

Choose a reason for hiding this comment

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

We don't have enough tests to verify that nothing will be broken, but it seems to be running fine on a manual build.

@@ -49,43 +46,26 @@
"dependencies": {
"@types/leaflet": "^1.9.12",
"@types/leaflet-draw": "^1.0.11",
"autocompleter": "^7.0.1",
"css-loader": "^7.1.2",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: css-loader is needed, but moving it to the devDependencies.

"lodash": "^4.17.21",
"native-promise-only": "^0.8.1",
"path-browserify": "^1.0.1",
"webpack": "^5.75.0"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: webpack is needed, but moving it to the devDependencies.

@nimya-aot
Copy link
Collaborator

I can run cypress tests on this env https://chefs-dev.apps.silver.devops.gov.bc.ca/pr-1554 if all these changes deployed there

@WalterMoar
Copy link
Collaborator Author

I can run cypress tests on this env https://chefs-dev.apps.silver.devops.gov.bc.ca/pr-1554 if all these changes deployed there

That would be great! The PR environment has just been updated with moving a couple of packages to the dev dependencies.

@nimya-aot
Copy link
Collaborator

@WalterMoar form.io components seems to be working . But a few of the tests are failing since my email id is not exist on Manage team section. Can someone add me to the CHEFS keycloak group on which PR env is running?
image

@WalterMoar
Copy link
Collaborator Author

@WalterMoar form.io components seems to be working . But a few of the tests are failing since my email id is not exist on Manage team section. Can someone add me to the CHEFS keycloak group on which PR env is running? image

I think that error message is saying that you need to log into the PR environment. That will create your user in CHEFS and then it will appear in the search.

@nimya-aot
Copy link
Collaborator

@WalterMoar Thanks for the info. Now its working good.

@WalterMoar WalterMoar merged commit 9924d53 into bcgov:main Dec 24, 2024
5 checks passed
@WalterMoar WalterMoar deleted the build/1679-remove-components-dependencies branch December 24, 2024 21:22
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.

3 participants