search

papandreou / express-processimage

Express middleware that processes served images according to the query string
BSD 3-Clause "New" or "Revised" License
56 stars 18 forks source link

Refresh development dependencies and use prettier 3 for formatting #202

Closed paras20xx closed 10 months ago

paras20xx commented 10 months ago

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 commented 10 months ago

Kindly review: @papandreou @alexjeffburke

github-actions[bot] commented 10 months ago

Coverage Status

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

papandreou commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

@papandreou Updated as per the suggestions :+1: :smile:

Note: Previously, package-lock.json was not generated via configuration in .npmrc. Now, it will be generated, but it is excluded via .gitignore.

npm doesn't support commented json files and hence usually to remember about pinning, I use "=" explicitly. Yes, this approach lacks automatic backported updates. Alternatively, for adding manual comments/notes, I created https://www.npmjs.com/package/package-cjson (not well documented) to use package.cjson and convert it to package.json :smile:

papandreou commented 10 months ago

Note: Previously, package-lock.json was not generated via configuration in .npmrc. Now, it will be generated, but it is excluded via .gitignore.

Please change it back so it isn't generated either. I really don't want it to show up in my local setup.

paras20xx commented 10 months ago

Note: Previously, package-lock.json was not generated via configuration in .npmrc. Now, it will be generated, but it is excluded via .gitignore.

Please change it back so it isn't generated either. I really don't want it to show up in my local setup.

Updated

paras20xx commented 10 months ago

All resolved now I believe :smile:

papandreou commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

Thank you @papandreou :smiley:

@alexjeffburke We'll wait in case you notice any breaking change :+1:

paras20xx commented 10 months ago

@alexjeffburke Is it fine to merge ?

@alexjeffburke @papandreou The only potential breaking change that I can think of is the change in "overrides" (https://github.com/papandreou/express-processimage/pull/202/commits/016707f04fb78b8b9082a426f0cf7e9eccfd2fb9). 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 commented 10 months ago

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 commented 10 months ago

@alexjeffburke Thank you for the update :smile:

alexjeffburke commented 10 months ago

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 commented 10 months ago

@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 :+1: 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 :+1:

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:

Does it sound fine?

alexjeffburke commented 10 months ago

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:

Thanks, Alex J Burke.

alexjeffburke commented 10 months ago

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.