Typescript 4.9.4 compatibility issue with catch(err) in Storage module

[wz2b]

Before opening, please confirm:

• I have searched for duplicate or closed issues and discussions.
• I have read the guide for submitting bug reports.
• I have done my best to include a minimal, self-contained set of instructions for consistently reproducing the issue.

JavaScript Framework

React

Amplify APIs

Authentication, Storage

Amplify Categories

auth, storage

Environment information

The npx envinfo command failed but the project configuration is important:

{
  "dependencies": {
    "@aws-amplify/auth": "^5.1.2",
    "@aws-amplify/core": "^5.0.8",
    "@aws-amplify/storage": "^5.0.8",
    "@aws-sdk/client-cognito-identity": "^3.238",
    "@aws-sdk/client-cognito-identity-provider": "^3.238.0",
    "@aws-sdk/credential-providers": "^3.238.0",
    "@babel/core": "^7.20.7",
    "@babel/plugin-syntax-flow": "^7.18.6",
    "@babel/plugin-transform-react-jsx": "^7.20.7",
    "@pmmmwh/react-refresh-webpack-plugin": "^0.5.10",
    "@svgr/webpack": "^6.5.1",
    "@testing-library/dom": "^8.19.1",
    "@testing-library/jest-dom": "^5.16.5",
    "@testing-library/react": "^13.4.0",
    "@testing-library/user-event": "^14.4.3",
    "@types/jest": "^29.2.4",
    "@types/node": "^18.11.18",
    "@types/react": "^18.0.26",
    "@types/react-dom": "^18.0.10",
    "babel-jest": "^29.3.1",
    "babel-loader": "^9.1.0",
    "babel-plugin-named-asset-import": "^0.3.8",
    "babel-preset-react-app": "^10.0.1",
    "bfj": "^7.0.2",
    "bootstrap": "^5.2.3",
    "browserslist": "^4.18.1",
    "camelcase": "^7.0.1",
    "case-sensitive-paths-webpack-plugin": "^2.4.0",
    "classnames": "^2.3.2",
    "css-loader": "^6.7.3",
    "css-minimizer-webpack-plugin": "^4.2.2",
    "dotenv": "^16.0.3",
    "dotenv-expand": "^10.0.0",
    "eslint": "^8.30.0",
    "eslint-config-react-app": "^7.0.1",
    "eslint-webpack-plugin": "^3.2.0",
    "file-loader": "^6.2.0",
    "fs-extra": "^11.1.0",
    "html-webpack-plugin": "^5.5.0",
    "identity-obj-proxy": "^3.0.0",
    "jest": "^29.3.1",
    "jest-resolve": "^29.3.1",
    "jest-watch-typeahead": "^2.2.1",
    "mini-css-extract-plugin": "^2.7.2",
    "postcss": "^8.4.20",
    "postcss-flexbugs-fixes": "^5.0.2",
    "postcss-loader": "^7.0.2",
    "postcss-normalize": "^10.0.1",
    "postcss-preset-env": "^7.8.3",
    "prompts": "^2.4.2",
    "react": "^18.2.0",
    "react-app-polyfill": "^3.0.0",
    "react-bootstrap": "^2.7.0",
    "react-bootstrap-icons": "^1.10.2",
    "react-dev-utils": "^12.0.1",
    "react-dom": "^18.2.0",
    "react-refresh": "^0.14.0",
    "react-router-dom": "^6.6.1",
    "resolve": "^1.22.1",
    "resolve-url-loader": "^5.0.0",
    "sass-loader": "^13.2.0",
    "semver": "^7.3.8",
    "source-map-loader": "^4.0.1",
    "style-loader": "^3.3.1",
    "tailwindcss": "^3.2.4",
    "terser-webpack-plugin": "^5.3.6",
    "typescript": "^4.9.4",
    "web-vitals": "^3.1.0",
    "webpack": "^5.75.0",
    "webpack-dev-server": "^4.11.1",
    "webpack-manifest-plugin": "^5.0.0",
    "workbox-webpack-plugin": "^6.5.4"
  },
  "eslintConfig": {
    "extends": [
      "react-app",
      "react-app/jest"
    ]
  },


}

{
  "compilerOptions": {
    "target": "es5",
    "lib": [
      "dom",
      "dom.iterable",
      "esnext"
    ],
    "allowJs": true,
    "skipLibCheck": true,
    "esModuleInterop": true,
    "allowSyntheticDefaultImports": true,
    "strict": true,
    "forceConsistentCasingInFileNames": true,
    "noFallthroughCasesInSwitch": true,
    "module": "esnext",
    "moduleResolution": "node",
    "resolveJsonModule": true,
    "isolatedModules": true,
    "noEmit": true,
    "jsx": "react-jsx",
    "useUnknownInCatchVariables": true
  },
  "include": [
    "src"
  ]
}

Describe the bug

When trying to build using Typescript 4.9.4, importing the @aws-amplify/storage module, I get this:

ERROR in node_modules/@aws-amplify/storage/src/common/S3ClientUtils.ts:100:32

TS18046: 'err' is of type 'unknown'.
     98 | 	} catch (err) {
     99 | 		if (isTimeSkewedError(err)) {
  > 100 | 			const serverDate = new Date(err.ServerTime);
        | 			                            ^^^
    101 | 			config.systemClockOffset = serverDate.getTime() - Date.now();
    102 | 		}
    103 | 		throw err;

Doing some research, this came in with Typescript 4.0 and is described HERE

the apparent mitigation is to change it to this:

} catch (err) {
   if(err instanceof Error) {
      if (isTimeSkewedError(err)) {
         const serverDate = new Date(err.ServerTime);
         config.systemClockOffset = serverDate.getTime() - Date.now();
      }
   }
   throw err;

Expected behavior

Expected it to compile without warnings

Reproduction steps

1. Install typescript 4.9.4
2. Try to use Storage.get()
3. Compile

Code Snippet

// Put your code below this line.
Storage.list( ... )

Log output

// Put your logs below this line

aws-exports.js

No response

Manual configuration

No response

Additional configuration

No response

Mobile Device

No response

Mobile Operating System

No response

Mobile Browser

No response

Mobile Browser Version

No response

Additional information and screenshots

No response

[nadetastic]

Hi @wz2b thank you for opening this issue - so far I haven't yet reproduced this, but I see that you opened #10828 - are these related/from the same app? Additionally, could you share some sample configuration of your setup?

[wz2b]

Hi there, the other one is more complicated. This one, however, is really in support of Typescript 4, so please correct me if I have something wrong here. The issue is described HERE but what I don't understand is why this hasn't been an issue before.

You can work around this by changing tsconfig.json to this:

"compilerOptions": {
    "useUnknownInCatchVariables": false
}

but as I understand it, the better long-term solution is to be explicit about your catches either by specifying any:

try {
    executeSomeThirdPartyCode();
}
catch (err: any) {
    console.error(err.message); // Works again!
}

or perhaps by leaving it unknown then testing the type. My gut feel is it's less intrusive to use catch(err: any) as that's less work and has no chance (that I can think of) to introduce any compatibility issues.

I think this is simpler to fix which is why I made it a separate issue. I will look for other instances where this is the case. Because I ran into the problem in #10828 maybe I can submit a P.R. but I'm not too familiar with this project and don't want to make a mistake.

[wz2b]

You asked about my project environment - sorry I forgot to answer. I'm building something using React, Typescript 4.94, and bootstrap. As usual, all those pieces bring in their own dependencies. For the most part, Yarn does a pretty good job of managing that. Since I'm using amplify-js I really don't need to bring in my own client-s3, and if I don't explicitly declare that as a dependency, it comes in as a sub-dependency at version 3.6.1. I'm not sure if that will cause conflicts with my other dependencies or not, but it always makes me nervous to use old dependencies in a brand new project. That makes me try to upgrade everything to latest, and that usually gets me into trouble! In this case, though, I thought I would try to leave everything as-is and just use amplify storage 5.0.8 and see what happens. I do want to use the most recent Typescript, though - Typescript gets better with every release.

Not to wax too philosophically but I try to write issues for which I can provide a standalone example, but extricating those examples out of a project is not always easy. But I will give it a try if you can't replicate this one. My advice though is to try to just build something - anything - using Typescript 4.9.4 and make sure in your tsconfig.json you have strict and alwaysStrict set to true, and either set useUnknownInCatchVariables to true or just don't specify it.

I'll try to extract an example without react. Meanwhile, if you do manage to replicate this, please comment here so I don't waste my time :-) I want to move on to the other problem.

[wz2b]

Question in THIS CODE:

catch (err) {
        if (isTimeSkewedError(err)) {
                const serverDate = new Date(err.ServerTime);
                config.systemClockOffset = serverDate.getTime() - Date.now();
        }
        throw err;
}

which is calling this:

const isTimeSkewedError = (err: any): boolean =>
        err.ServerTime &&
        typeof err.Code === 'string' &&
        err.Code === 'RequestTimeTooSkewed';

That's assuming that the error is some type that has ServerTime and Code fields. Is there a type associated with whatever that is? Does one need to be created?

[wz2b]

I was hoping to find a solution to this problem that worked with both typescript 4 (current) and typescript 3 (what this library was built using). I am quickly coming to the conclusion that isn't possible for a few reasons:

• Under TS 3 you aren't allowed to specify the type of thing you are catching (TS1196)
• Under TS 4 you must specify the type as 'unknown' or 'any', or let it default to 'unknown' and provide type guards
• We can't really provide type guards because the error that comes back doesn't actually have a type

I think the solution to this problem is to upgrade the whole library to Typescript 4, but chances are excellent that will break a lot. Some of what it will break is IMO already broken (see #10828) at least in terms of typing (it might actually work because the types overlap enough, so if you don't have strict type checking you get away with it).

The only other way I can think of fixing this would be a type guard before callling isTimeSkewedError() but the problem is the error doesn't seem to have a type.

I don't feel like upgrading the entire library is something I should undertake. I am going to stop working on this just until contributors who are closer to the project give me some guidance and discussion.

[israx]

Hello @wz2b . Thank you for taking the time on investigating about this issue.

Checking internally, we need to update the isTimeSkewedError type guard function. This would work with any typescript version.

[wz2b]

Hello @wz2b . Thank you for taking the time on investigating about this issue.

Checking internally, we need to update the isTimeSkewedError type guard function. This would work with any typescript version.

That sounds good, assuming the compiler is smart enough to know that yes, it is type guarded, just further down the call stack. I will keep my eyes open and when you push a commit for this I'll help test with 4.9.4 and my tsconfig.json (meaning leave strict and alwaysStrict on and don't bypass catch typing rules)

[israx]

Hi @wz2b. I failed trying reproducing this issue with a react app using the same dependencies and tsconfig.json settings.

However updating this isTimeSkewedError type guard is something that we are looking to improve, as we are potentially planning on upgrading the TS version

[israx]

Hello @wz2b . I'm trying to reproduce this issue to understand how it would affect other customers. Can you elaborate a little bit more on how you were able to get that error ?

[wz2b]

Hello @wz2b . I'm trying to reproduce this issue to understand how it would affect other customers. Can you elaborate a little bit more on how you were able to get that error ?

I don't have any more information but you seemed to understand the problem in a previous comment. The summary is this: in Typescript 4, the argument to catch() is of type any or unknown, depending on options flags and whether or not strict is set. No matter what the options, this library shouldn't dictate these settings. It's easy to avoid the problem by just being explicit about types.

In your case, what happens is that you send the error type (any or unknown) on to isTimeSkewedError() without any type guarding.

https://devblogs.microsoft.com/typescript/announcing-typescript-4-4/#use-unknown-catch-variables describes the problem very well, I'm not sure I have anything to add to it, and I'm not sure why you can't reproduce it.

[israx]

Yes, we understand the issue is caused by typescript not being upgraded to a grater version and not having the right type guard.

However using the same dependencies and tsconfig.json settings on a react app is not triggering that issue on my end. What developing tool are you using to create your app (npx, vite )?

[jimblanc]

We have published an RFC on our plan for improving TypeScript support in Amplify JS & would love to get your feedback & suggestions!

RFC: Amplify JS TypeScript Improvements

[cwomack]

The developer preview for v6 of Amplify has officially been released with improvements to TypeScript support and much more! Please check out our announcement and updated documentation to see what has changed.

This issue should be resolved within the dev preview and upcoming General Availability for Amplify v6, but let us know with a comment if there are further issues.

[cwomack]

With the release of the latest major version of Amplify (aws-amplify@>6), this issue should now be resolved! Please refer to our release announcement, migration guide, and documentation for more information.