Refresh development dependencies and use prettier 3 for formatting

[paras20xx]

Also adds a 'Sample guide for updating image test data' in test/processImage.js.

Changes related to changing the sharp version which requires a major version bump moved to separate PR.

[paras20xx]

Kindly review:
@papandreou @alexjeffburke

[github-actions]

coverage: 97.59%. remained the same
when pulling c2ca220 on general-maintenance
into c89abae on master.

[papandreou]

Hey Priyank, long time no see! This looks fine.

Could you leave out the addition of package-lock.json? It causes a lot of dependency churn and doesn't really make sense to use in libraries, as it isn't honored when installing the library as a dependency.

I guess the fact that the new version of exif-reader changes the names of tags would make this a major update?

[paras20xx]

@papandreou
Hi there :-)

Thank you for the quick review.

You mentioned "dependency churn". What is that in relation with package-lock.json here ? Can you point me for something to search for about it ?

The package-lock.json doesn't get published, but would be useful for further development and reproducibility. Also, without package-lock.json the npm audit wouldn't work. So, no disadvantages of keeping it.

Regarding exif-reader, I hadn't noticed that change. I'll check our current use-cases of this library as well to identify if that is breaking. Anyways, making the next version as major would be safer then.

Regarding pinning to specific versions, the reason is that the newer versions of those packages are marked as "type": "module" and to get those to work would lead to bigger refactoring. Hence marked the pinning explicitly.

Also looking for approval from @alexjeffburke since he has recently worked on some changes and kept Node.js 16 support which I had to drop for sharp library (it seemed to be causing some issue with Node.js 16).

[papandreou]

The package-lock.json doesn't get published, but would be useful for further development and reproducibility. Also, without package-lock.json the npm audit wouldn't work. So, no disadvantages of keeping it.

I disagree, it's not worth the extra churn. Please revert.

[paras20xx]

All resolved now I believe 😄

[papandreou]

Looks good to me now! Happy to merge it as-is, but you're welcome to wait and see if Alex has time to review it also.

[alexjeffburke]

Hi, Thanks for the ping. There’s a few things here we probably need to circle over, but we’ll have to pick discussions up Monday/Tuesday in the coming week :)

[paras20xx]

Thank you @papandreou 😃

@alexjeffburke
We'll wait in case you notice any breaking change 👍

[paras20xx]

@alexjeffburke
Is it fine to merge ?

@alexjeffburke @papandreou
The only potential breaking change that I can think of is the change in "overrides" (016707f). If you find it risky, then that can be left out and can be manually handled by a project using express-processimage (if required).

[alexjeffburke]

Hi,

I’m sorry for the delay, I’ve actually been under the weather with a nasty cold.

I’ve been drafting a response to you already thjs morning, please hold off until we can have that discussion.

Thanks, Alex J Burke.

[paras20xx]

@alexjeffburke
Thank you for the update 😄

[alexjeffburke]

Hi,

Let me say up-front I really appreciate your spending time on this. As a general rule, we try to make individual changes in separate changes rather mix things together like this. For example, you've bumped prettier and thus a bunch of reformatting is in here as well as the bump of different versions. I would like to see the prettier bump and formatting stuff in a separate PR.

So, a little bit of background. Originally express-processimage contained its own code for processing images but as of version 9 al the image processing is handled by impro which is a much evolved version of that original code. As such, beyond the middleware part of this module it is really a "distribution" of impro since it hard depends on a bunch of libraries that will be loaded by impro at import time.

What this has meant practically is, at least on my part, trying to keep the versions exposed here in sync with what is supported by impro. From my perspective it does not make sense to ship this with versions that may not be compatible with the thing actually orchestrating the hard image processing work. In addition, as a maintainer here and an author of much of impro, it has also been a way to avoid an additional maintenance burden that could result from a bunch of different versions of things running in the wild.

Given the above, first question: have you verified that the sharp version you're pulling in here is fully compatible with impro, for example by running the impro test suite against that version?

Secondly, there does seems to be a lot of churn in this PR. The versions of libraries were specifically defined with carets and tildes to make sure that the latest patch versions of them are always installed. For example, you've changed inkscape ^3.0.0 to ^3.1.1, but that won't change what is installed. I've generally kept those numbers as the minimal versions required for the library to function, so could you elaborate on the need to change them when it leads to no functional difference?

Thirdly, you mentioned vulnerabilities and I must push back rather strongly against that: production installs of this module and its dependencies generate no audit warnings at all, and I actively made quite sure of that when I did the last release (which was v11 on the 18th December) so the suggestion of critical vulnerabilities when audit is reporting on dev dependencies isn't warranted.

That is probably enough questions to start with, so let's take it from there :)

Thanks, Alex J Burke.

[paras20xx]

@alexjeffburke

While there are multiple changes in the same PR, they were broken into separate commits with each commit doing something of its own. If that approach doesn't work, I can cherry-pick those changes accordingly and submit them as separate PRs 👍
I took that approach since I wasn't sure which all things might need to be changed or which things might lead to merge conflicts.

That merge conflict scenario is more common for cases where package-lock.json is involved (which I initially started with) but then I removed the package-lock.json as asked by @papandreou

Regarding impro and its compatibility with sharp, let me check in that repository (and raise a PR if required).

Regarding updating minor versions (eg: inkscape ^3.0.0 to ^3.1.1), I attempted to mark the latest versions of the package wherever possible. This approach ensures that the application which is consuming it also installs the at least that version. Otherwise, there is an edge-case scenario as per npm's algorithm that if someone has installed eg: v3.0.0 previously, if the user doesn't remove (the respective node_modules and/or the old package-lock.json file in their project), then it may not necessarily update to 3.1.1 and would attempt to reuse the older version. And hence, it is possible that the consuming project would miss out on minor/patch fixes.

Personally, to avoid this edge case scenario, in my projects which consume various npm packages, I remove package-lock.json and node_modules/ folder and do npm install again from time-to-time.

Since it is a workaround-able thing, I can update the MR / raise separate MR to fix that 👍

Regarding the vulnerabilities, I don't remember if I had checked it for dependencies only or included devDependencies as well. Anyways, it doesn't need to be the responsibility of this library and can be handled by the consuming project. I'll remove that part.

The primary purpose for my PR was to identify/fix any direct/indirect security vulnerabilities. The approach I took was to update all the dependencies to their latest versions (and handle related changes). Which would be followed by any other updates, if required, in the project which is consuming this library.

Action points from my end:

• Split the change into multiple PRs (@alexjeffburke kindly confirm if that is required)
• Check the compatibility of the latest version of sharp w.r.t. impro and take appropriate action. (Please see: General maintenance and npm package updates (please see mentioned details) impro#7)
• Use lowest version with ^ or ~ sign in package.json (@alexjeffburke kindly confirm if that is required, though my opinion is to use the latest compatible version there)
• Remove the "overrides" change. (Updated with 4482960)

Does it sound fine?

[alexjeffburke]

Hi,

Let's get to that actionable point you mentioned. Comments inline and will close with some agreements.

@alexjeffburke

While there are multiple changes in the same PR, they were broken into separate commits with each commit doing something of its own. If that approach doesn't work, I can cherry-pick those changes accordingly and submit them as separate PRs 👍 I took that approach since I wasn't sure which all things might need to be changed or which things might lead to merge conflicts.

That merge conflict scenario is more common for cases where package-lock.json is involved (which I initially started with) but then I removed the package-lock.json as asked by @papandreou

@papandreou wisely requested that removal and I wholeheartedly support that. Package locking is for applications to pin dependencies to specific versions for the needs of the application (for example pinning to the specific version of something for reasons needed by the consumer). We're a library and thus have different needs/constraints :)

Regarding impro and its compatibility with sharp, let me check in that repository (and raise a PR if required).

For the record, I didn't request any changes to versioning in impro, and for transparency I can't accept that PR you opened, at least as-is. To clarify my request: I wanted to make sure that, since you're proposing a sharp version bump, we validate that the image pipeline is actually compatible with what you want to land here. Can you confirm that, modulo any slight differences in test image output, that sharp 0.33.x works without issue in impro?

Regarding updating minor versions (eg: inkscape ^3.0.0 to ^3.1.1), I attempted to mark the latest versions of the package wherever possible. This approach ensures that the application which is consuming it also installs the at least that version. Otherwise, there is an edge-case scenario as per npm's algorithm that if someone has installed eg: v3.0.0 previously, if the user doesn't remove (the respective node_modules and/or the old package-lock.json file in their project), then it may not necessarily update to 3.1.1 and would attempt to reuse the older version. And hence, it is possible that the consuming project would miss out on minor/patch fixes.

Yep. As I mentioned above, this a difference between versioning as regards libraries versus applications. The default npm behaviour of installing the latest semver compatible versions is for libraries where we specify compatible versions while the exact installed versions via locking etc is for applications. Managing lock files is best left to application authors.

Personally, to avoid this edge case scenario, in my projects which consume various npm packages, I remove package-lock.json and node_modules/ folder and do npm install again from time-to-time.

Since it is a workaround-able thing, I can update the MR / raise separate MR to fix that 👍

Regarding the vulnerabilities, I don't remember if I had checked it for dependencies only or included devDependencies as well. Anyways, it doesn't need to be the responsibility of this library and can be handled by the consuming project. I'll remove that part.

The primary purpose for my PR was to identify/fix any direct/indirect security vulnerabilities. The approach I took was to update all the dependencies to their latest versions (and handle related changes). Which would be followed by any other updates, if required, in the project which is consuming this library.

Action points from my end:

* Split the change into multiple PRs (@alexjeffburke kindly confirm if that is required)

* Check the compatibility of the latest version of sharp w.r.t. impro and take appropriate action. (Please see: [General maintenance and npm package updates (please see mentioned details) impro#7](https://github.com/papandreou/impro/pull/7))

* Use lowest version with ^ or ~ sign in package.json (@alexjeffburke kindly confirm if that is required, though my opinion is to use the latest compatible version there)

* Remove the "overrides" change. (Updated with [4482960](https://github.com/papandreou/express-processimage/commit/4482960e5812c937e018ff9bdcec1d8729855e3b))

Does it sound fine?

Yeah, that sounds like a fine list. To avoid confusion, let me independently enumerate what I'm hoping to see:

• a separate PR containing the sharp version bump, associated test image changes and drop of node 16). Suggested title: "upgrade sharp to 0.33 and drop node 16 support (semver major)"
• restoring the minimal version requirements in package.json (I will follow-up by marking all those that need resetting)

Thanks, Alex J Burke.

[alexjeffburke]

Hi there,

I've taken the liberty to make the changes restoring the versions that we discussed and have thus been able to squash and merge the other bits.

I've moved the sharp version bump and associated major version bump into its own branch - sharp-0.33. I am keen to keep attribution of the work, so would like invite you to open a PR for that branch @paras20xx

Thanks, Alex J Burke.