Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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
3 tasks done
bogdanailincaipnt opened this issue Aug 27, 2024 · 6 comments
Assignees
Labels
bug This issue is a bug. p2 This is a standard priority issue response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days.

Comments

@bogdanailincaipnt
Copy link

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: aws-amplify/amplify-js#11736

SDK version number

@aws-sdk/[email protected]

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 aws-amplify/amplify-js#11736

Expected Behavior

See aws-amplify/amplify-js#11736

Possible Solution

No response

Additional Information/Context

No response

@bogdanailincaipnt bogdanailincaipnt added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 27, 2024
@zshzbh zshzbh self-assigned this Aug 29, 2024
@zshzbh
Copy link

zshzbh commented Aug 29, 2024

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 zshzbh added p2 This is a standard priority issue investigating Issue is being investigated and/or work is in progress to resolve the issue. and removed needs-triage This issue or PR still needs to be triaged. labels Aug 29, 2024
@zshzbh
Copy link

zshzbh commented Aug 29, 2024

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

@zshzbh zshzbh added response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. and removed investigating Issue is being investigated and/or work is in progress to resolve the issue. labels Aug 29, 2024
@bogdanailincaipnt
Copy link
Author

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
Copy link
Contributor

kuhe commented Aug 30, 2024

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
Copy link
Author

I see a lot of people are having this problem with other packages as well. microsoft/TypeScript#37053

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

@bogdanailincaipnt
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. p2 This is a standard priority issue response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days.
Projects
None yet
Development

No branches or pull requests

3 participants