Skip to content
This repository was archived by the owner on Dec 9, 2024. It is now read-only.

fix: remove unused semver dependency #668

Merged
merged 2 commits into from
May 25, 2023
Merged

fix: remove unused semver dependency #668

merged 2 commits into from
May 25, 2023

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented May 20, 2023

What did you implement:

The semver package is no longer being used, so it doesn't need to be listed as a dependency.

How did you implement it:

I removed it from dependencies in package.json

How can we verify it:

❯ rg --files-with-matches 'semver'
integrationTests/configurations/node16-linux-webpack/package-lock.json
integrationTests/configurations/node12-linux/package-lock.json
integrationTests/configurations/node18-linux-webpack/package-lock.json
integrationTests/configurations/node14-windows/package-lock.json
integrationTests/configurations/node16-linux/package-lock.json
integrationTests/configurations/node12-linux-webpack/package-lock.json
integrationTests/configurations/node16-windows-webpack/package-lock.json
integrationTests/configurations/node14-windows-webpack/package-lock.json
integrationTests/configurations/node18-windows-webpack/package-lock.json
integrationTests/configurations/node16-windows/package-lock.json
integrationTests/configurations/node18-windows/package-lock.json
integrationTests/configurations/node12-windows-webpack/package-lock.json
integrationTests/configurations/node12-windows/package-lock.json
integrationTests/configurations/node14-linux-webpack/package-lock.json
integrationTests/configurations/node14-linux/package-lock.json
integrationTests/configurations/node18-linux/package-lock.json
examples/typescript-webpack/package-lock.json
package-lock.json

If semver was being used by this package, then there should have been a match in either a TypeScript or JavaScript file, which there was not.

The tests also will fail if semver is actually needed.

Todos:

Note: Run npm run test:ci to run all validation checks on proposed changes

  • Ensure there are no lint errors.
    Validate via npm run lint
    Note: Some reported issues can be automatically fixed by running npm run lint:fix
  • Write tests and confirm existing functionality is not broken.
    Validate via npm test
  • Write documentation
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@G-Rath
Copy link
Contributor Author

G-Rath commented May 20, 2023

It looks like acorn is also completely unused, and that a single lodash function is being used in a test file. It also looks like azure-functions-core-tools is unused but actually it's been called via spawn.

@gligorkot I'm happy to do PRs removing acorn and at least marking lodash as a dev dependency if not removing it completely. I've opened #669 and #670 too; these will possibly conflict with each other as they're merged, but I'm happy to handle rebasing them.

@gligorkot
Copy link
Contributor

Sounds good @G-Rath, I'll have a look at these a bit later today. It would be great if we can eliminate any unused dependency and then upgrade all of the dependencies and write tests for all the functionality. Then we could release a v3.0.0 which would be a major step forward to keeping this plugin up to date with the latest web technologies

@G-Rath
Copy link
Contributor Author

G-Rath commented May 21, 2023

@gligorkot awesome - fully support doing some dependency upgrades, this plugin is a bit behind the times 😅

I've had a look over the dependencies and have a few suggestions if you'd like them.

@gligorkot
Copy link
Contributor

@G-Rath please! Unfortunately, I've got a bit on over the next few weeks at work so probably won't put in any time into the upgrades, but feel free to start a branch and I can join in and contribute as well when able to.

Also, if you can add to the tests as we're upgrading things that would be great too.

@G-Rath
Copy link
Contributor Author

G-Rath commented May 21, 2023

@gligorkot unfortunately I'm pretty busy myself right now, but I'll see what I can do. If we can get a couple of the prs I've opened so far that'd help because then I won't need approval for the CI workflows to run.

I'll post a write-up of my thoughts on each dependency sometime this week

@gligorkot gligorkot merged commit 002818c into serverless:master May 25, 2023
@G-Rath G-Rath deleted the remove-semver branch March 28, 2024 02:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants