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

G-Rath commented 3 years ago

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 commented 3 years ago

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

medikoo commented 3 years ago

@tbarlow12 can you check that one?

G-Rath commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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

G-Rath commented 3 years ago

@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 commented 2 years ago

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

G-Rath commented 2 years ago

@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 commented 1 year ago

@gligorkot I see that you're helping maintain this plugin now - would you be open to landing this if I rebased it?

gligorkot commented 1 year ago

@G-Rath sure, just need to understand it a bit more. Are you saying that webpack is required for this package so it needs to be a peer dependency? Also, why also make serverless webpack a peer dependency in that case too?

G-Rath commented 1 year ago

@gligorkot neither packages are required for this plugin, but if serverless-webpack is present then this plugin will utilize that.

The version of serverless-webpack being used at the time was v4 so this plugin is considered compatible with only that major version, which is why it is specified as a peer dependency (or, should have been).

I'm pretty sure that there isn't a need to also specify webpack as a peer dependency but ultimately imo the most important thing is that the configuration right now is leading to these packages being installed regardless of if you actually use them since they're incorrectly marked as optionalDependencies, and they're old versions with known security vulnerabilities - I would definitely support reviewing if webpack should be removed entirely as a dependency.

gligorkot commented 1 year ago

@G-Rath my plan is to work on updating all of the dependencies to their latest and then releasing a v3.0.0. However, regarding the discussion about whether or not webpack and serverless webpack are necessary to be peer or optional dependencies, if you use npm install --no-optional that should install without the optional dependencies, doesn't it?

I'm still not clear how marking them as peer dependencies changes anything - if they're marked as peer dependencies then you wouldn't be able to install this plugin without installing those dependencies as well, right? (not sure if anyone uses this plugin without them as I'm new to the codebase and hadn't been involved in any discussions about it before)

G-Rath commented 1 year ago

if you use npm install --no-optional that should install without the optional dependencies, doesn't it?

Technically yes, but it'll not install any optional dependencies which is not desirable - it also requires you to explicitly pass that whenever you run an npm command to ensure the dependencies are never installed.

if they're marked as peer dependencies then you wouldn't be able to install this plugin without installing those dependencies as well, right?

No - as I said, neither package is required for this plugin to work.

Optional dependencies are for situations where a particular package is desirable to have but may not be able to be installed for external reasons such as incompatible OS - fsevents being the classic example: it's a library for providing native access to the FSEvents API, which is a macOS specific API for watching files for changes; this is used by chokidar because as an efficient cross-platform file watching library, fsevents is the most efficient way to do file watching on macOS - however because fsevents is macOS only chokidar specifies it within optionalDependencies so that chokidar can still be installed on non-macOS devices (since chokidar is not OS specific).

Peer dependencies on the other hand are for situations where a particular package is required but there should only be one version of it in the dependency tree, generally because of the nature of the work it does and the way the package is invoked, and because of how Node supports multiple versions of the same package. Consider for example if you have an app using webpack v4, and you install a plugin that was built for v3 - if that plugin specified webpack within dependencies, your package manager would install webpack v3 as a nested dependency (i.e. node_modules/my-webpack-plugin/node_modules/webpack successfully, but when you ran npx webpack it would error because that would invoke node_modules/webpack. But if the plugin instead specified webpack within peerDependencies, the package manager would see that v4 was installed at the top of the tree and error (or warn depending on the package manager) because that is not compatible with v3.

Finally, we have optional peer dependencies - these are similar to the above, except that they don't require the peer dependency; they're for saying "this package supports enhanced behaviour if this package is installed, but it isn't required for the package to run". This plugin is an example of that: if serverless-webpack is listed as a plugin in the serverless config, then this plugin will do some additional behaviour to optimize the usage of serverless-webpack, but it will function perfectly fine if it's not (and importantly, this plugin does not behaviour differently just because the package is present on the tree - it has to be opted into by a config file).

You can also see this project as proof that this plugin can be used without these dependencies being configured.

gligorkot commented 1 year ago

Yeah, I'm happy to bring in this change

serverless / serverless-azure-functions