serverless-azure-functions icon indicating copy to clipboard operation
serverless-azure-functions copied to clipboard

fix: mark `webpack` dependencies as peer instead of optional

Open G-Rath opened this issue 4 years ago • 8 comments

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 via npm run lint
    Note: Some reported issues can be automatically fixed by running npm run lint:fix
  • [x] Write tests and confirm existing functionality is not broken.
    Validate via npm 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).

G-Rath avatar Dec 20 '20 00:12 G-Rath

@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 :)

G-Rath avatar Jan 17 '21 17:01 G-Rath

@tbarlow12 can you check that one?

medikoo avatar Jan 18 '21 14:01 medikoo

@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 :)

G-Rath avatar Feb 11 '21 19:02 G-Rath

@medikoo @tbarlow12 again any update? this is still a big problem for us - it's pretty much turned us off azure for now.

G-Rath avatar Mar 30 '21 04:03 G-Rath

@tbarlow12 let me know if you'd like me to rebase this so it can be landed.

G-Rath avatar May 18 '21 22:05 G-Rath

@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 :)

G-Rath avatar Jun 14 '21 05:06 G-Rath

@medikoo would it be possible to get this reviewed & merged? I'm also open to helping maintain this package if you want.

G-Rath avatar Dec 22 '21 23:12 G-Rath

@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

G-Rath avatar Sep 03 '22 23:09 G-Rath