aws / aws-sdk-js-v3

Modularized AWS SDK for JavaScript.
Apache License 2.0
3.11k stars 578 forks source link

Installing @aws-sdk/core or @aws-sdk/types tells TS that NodeJS globals are available, which can make TS ignore potential errors in browsers #6413

Open bogdanailincaipnt opened 2 months ago

bogdanailincaipnt commented 2 months ago

Checkboxes for prior research

Describe the bug

Installing @aws-sdk/core or @aws-sdk/types tells TS that NodeJS globals are available, which can make TS ignore potential errors in browsers.

Related issue and more info, context: https://github.com/aws-amplify/amplify-js/issues/11736

SDK version number

@aws-sdk/core@3.635.0

Which JavaScript Runtime is this issue in?

Browser

Details of the browser/Node.js/ReactNative version

Chrome: 128.0.6613.84

Reproduction Steps

https://stackblitz.com/edit/vitejs-vite-gmzgbg?file=src%2Fmain.ts

  1. Wait for npm install to finish.
  2. Note that when hovering on process.env, TS shows us that NodeJS globals are available.
  3. If you comment out the line that does import '@aws-sdk/core';, the errors show up. They should show up either way, because I've specified "types": [], in tsconfig.json, so global types from @types/node should not be included, even though I've installed the @types/node package.

Observed Behavior

See https://github.com/aws-amplify/amplify-js/issues/11736

Expected Behavior

See https://github.com/aws-amplify/amplify-js/issues/11736

Possible Solution

No response

Additional Information/Context

No response

zshzbh commented 2 months ago

Hey @bogdanailincaipnt ,

Thanks for the feedback!

I can reproduce this issue and this issue also exists in other AWS clients.

For example I can see the issue also exists in @aws-sdk/client-s3

import {S3Client} from "@aws-sdk/client-s3";
console.log(S3Client);

process.env = {};
console.log(process.env);
export {};

From TS doc -

Specify "types": [] to disable automatic inclusion of @types packages.

Keep in mind that automatic inclusion is only important if you’re using files with global declarations (as opposed to files declared as modules). If you use an import "foo" statement, for instance, TypeScript may still look through node_modules & node_modules/@types folders to find the foo package.

So this is expected that TS would still look through node_modules & node_modules/@types folders to find the aws package even though you put "types": [], in tsconfig.json.

I will bring this issue to the team to see if there's anything we can do.

Thanks! Maggie

zshzbh commented 2 months ago

Hey @bogdanailincaipnt ,

Just got back from the team-

It seems that any package written (with node like we do) will probably face this problem in the browser, we can't really avoid that since we do need to use node APIs/internals. we also offer the browser variant (if the bundler understands the relevant metadata)

A workaround idea would be - put business logic in a file/folder and import the aws specific stuff in a separate file.

We also want to check if the build fails on your end? If the build fails then it would be a high priority task for AWS SDK team.

Thanks! Maggie

bogdanailincaipnt commented 2 months ago

Hey @zshzbh , fortunately the build does not fail on my end.

I am using Vite as the bundler. Should I not be having this issue ? Note that I am actually using the aws-amplify package in my project.

Thanks! Bogdan

kuhe commented 2 months ago

This SDK is written for Node.js and offers browser compatibility via bundler metadata in package.json for file replacement.

I believe the only way to remove Node.js global types is to make all platform-specific code injectable. This would change the basic API contract that most users are accustomed to, so we are not planning on doing it.

As a possible alternative, Biome and eslint offer noRestrictedGlobals to help you catch errors.

bogdanailincaipnt commented 2 months ago

I see a lot of people are having this problem with other packages as well. https://github.com/microsoft/TypeScript/issues/37053

There are some ideas here https://github.com/mobxjs/mobx/pull/3583 but I don't know if they can be used for aws-sdk.

bogdanailincaipnt commented 2 months ago

There's also a potential solution for me to use https://github.com/ds300/patch-package and remove all occurences of /// <reference types="node" /> from node_modules. But this feels like something that could also be done in the aws-sdk repo, and it would make more sense there I think.