AWS SDK jsv3 migration

[VadimKovalenkoSNF]

Superseeds #1851
Fixes #1840

1. Migrate S3 module to aws sdk for js v3.
2. Add proper error handling.
3. Refactor S3 unit tests.

[VadimKovalenkoSNF]

This PR expect for CI fix here: #1862

[codecov]

Codecov Report

Patch coverage has no change and project coverage change: -0.56% ⚠️

Comparison is base (040b9c6) 70.29% compared to head (7e65e0d) 69.73%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1865      +/-   ##
==========================================
- Coverage   70.29%   69.73%   -0.56%     
==========================================
  Files          24       24              
  Lines        2626     2647      +21     
  Branches      596      601       +5     
==========================================
  Hits         1846     1846              
- Misses        667      690      +23     
+ Partials      113      111       -2     

Files Changed	Coverage Δ
src/S3.ts	0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kelson42]

LGTM (at high level) and seems to work but I'm worried about two new errors during npm i which seem to have appeared with this PR:

> mwoffliner@1.13.0 build
> ./dev/build.sh

Building at [Do 27 Jul 2023 20:34:36 CEST]
node_modules/@types/imagemin-gifsicle/index.d.ts:7:24 - error TS1479: The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("imagemin")' call instead.

7 import { Plugin } from 'imagemin';
                         ~~~~~~~~~~

node_modules/@types/imagemin-webp/index.d.ts:7:24 - error TS1479: The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("imagemin")' call instead.

7 import { Plugin } from 'imagemin';
                         ~~~~~~~~~~


Found 2 errors in 2 files.

Errors  Files
     1  node_modules/@types/imagemin-gifsicle/index.d.ts:7
     1  node_modules/@types/imagemin-webp/index.d.ts:7
Build Complete at [Do 27 Jul 2023 20:34:48 CEST]

Can we fix them?

I'm surprised the CI goes through with such a severe error during the installation process. Wouldn't that be appropriate to die/stop the CI on such scenarios?

[kelson42]

@VadimKovalenkoSNF Please fix the rebase conflict as well.

[VadimKovalenkoSNF]

TypeScript error (TS1479) highlighted here: microsoft/TypeScript#54523

[kelson42]

@VadimKovalenkoSNF Do you want me to redo the review? If "yes" please request a new review on Github (see the icon for that on Github). Otherwise, why the codecov goes back? Is the PR fully tested (then it should not go back!)?

[VadimKovalenkoSNF]

I'll try to increase test coverage here.

[VadimKovalenkoSNF]

@kelson42 For some reason codecov marks almost everything in S3 module as uncovered, see https://app.codecov.io/gh/openzim/mwoffliner/pull/1865/blob/src/S3.ts
But methods for this class has been already tested here

[kelson42]

@VadimKovalenkoSNF We better clarify this... otherwise no way to trust codecov. Maybe the code coverage command does not take in account all the tests? Check with package.json and `ci,yml'

[VadimKovalenkoSNF]

@kelson42 I noticed that test coverage of S3 equals to zero during build stage, see - https://github.com/openzim/mwoffliner/actions/runs/5693400834/job/15432418757#step:7:622

I noticed the same issue in another PR, check - https://github.com/openzim/mwoffliner/actions/runs/5684250388/job/15406524636#step:7:677

[VadimKovalenkoSNF]

@kelson42 I noticed that test coverage of S3 equals to zero during build stage, see - https://github.com/openzim/mwoffliner/actions/runs/5693400834/job/15432418757#step:7:622

I noticed the same issue in another PR, check - https://github.com/openzim/mwoffliner/actions/runs/5684250388/job/15406524636#step:7:677

Upd: I was wrong, it seems that the correct report related to S3 is here - https://github.com/openzim/mwoffliner/actions/runs/5693400834/job/15432418757#step:7:221

[VadimKovalenkoSNF]

I compared test coverage for S3 between main and aws-sdk-jsv3-migration branch.

Git branch	File	% Stmts	% Branch	% Funcs	% Lines
aws-sdk-jsv3-migration report	S3.ts	81.25	78.57	86.36	81.25
main report	S3.ts	82.35	71.42	93.75	82.35

As you can see, function coverage decreased, but this happened because I've introduced a private method setRegion() to get a region of S3 bucket (this wasn't needed in the prev aws-sdk version). I did so to hide implementation details, but on the other hand, test coverage for conditions increased. Given this information, I assume, that this PR can be approved.