serverless-azure-functions
serverless-azure-functions copied to clipboard
fix: mark `webpack` dependencies as peer instead of optional
What did you implement:
When webpack was added as a dependency in #105 it was added as an "optional" dependency using optionalDependencies
, when it should have been in peerDependencies
.
This is a common mistake, as optionalDependencies
sounds like what you want when a dependency is optional, but those dependencies mean "if you error when installing this, that's fine, ignore it and continue" - they're used for packages like fsevents
which are only supported in some OSs (or other condition).
peerDependencies
on the other hand mean "I support working with this version of this package , but require a single version of it to exist in the tree". Taking this even further, peerDependenciesMeta
lets you mark a package as really optional (aka "I support using this package within this version constraint, but I don't require it to function"); otherwise a peer dependency will be installed by default (maybe, depending on package manager - it use to be a thing in npm
then they removed that for 5 & 6 but added it back in 7).
Doing this lets us not pull in the old version of webpack that has known vulnerabilities if we're not wanting to use webpack. (Upgrading the version of webpack supported by this library is another thing of it's own 😬).
Currently this is pulling in an extra 155 packages (including at least 6 known vulnerabilities that cannot be patched due to webpack 3),
How did you implement it:
By adjusting the package.json
.
How can we verify it:
Do npm i serverless-azure-functions && npm ls webpack
- you'll see its currently being installed in the tree, but there are no requires for it in code.
If you do npm i
on this branch, you'll see webpack not on the tree and ~150 packages are removed.
Todos:
Note: Run npm run test:ci
to run all validation checks on proposed changes
- [x] Ensure there are no lint errors.
Validate vianpm run lint
Note: Some reported issues can be automatically fixed by runningnpm run lint:fix
- [x] Write tests and confirm existing functionality is not broken.
Validate vianpm test
- [x] Write documentation
- [x] Provide verification config / commands / resources
- [x] Enable "Allow edits from maintainers" for this PR
- [x] Update the messages below
Is this ready for review?: YES Is it a breaking change?: NO (sort of)
This is in the annoying grey area of "is this is a breaking change" - if people are relying on the fact that serverless-webpack
is being installed via serverless-azure-functions
then this will break their build, but they're relying on a subtle functionality of package management that shouldn't be relied on to begin with (as it can itself break without warning), and the fix is entirely just "run npm i -D serverless-webpack webpack
.
Users that have serverless-webpack
in their package.json
as a dependency won't be affected - things will just continue to work for them (they'll probably have their dependency tree shrink tbh).
@pgrzesik @medikoo would it be possible to get some eyes on this?
I hate to ping out-of-the-blue, but this is preventing us from auditing our projects, and pulls in a lot of outdated and unneeded dependencies with no way to work around 😬
Happy to do the leg work, just would like to spur some movement on the review side to get this landed :)
@tbarlow12 can you check that one?
@medikoo @tbarlow12 any update on this? I hate to keep pinging, but as I've said it's pretty major for us.
I'm happy to help if there's anything I can do :)
@medikoo @tbarlow12 again any update? this is still a big problem for us - it's pretty much turned us off azure for now.
@tbarlow12 let me know if you'd like me to rebase this so it can be landed.
@tbarlow12 this needs rebasing because #537 changed the lockfile from v1 to v2 (meaning the author was using npm
v7) - I'm happy to rebase my branch if you're keen to merge this :)
@medikoo would it be possible to get this reviewed & merged? I'm also open to helping maintain this package if you want.
@medikoo @tbarlow12 bump - it would be really great if we could get this landed. Rebasing will be easy becuase it's just that the package lock needs to be regenerated