aws-amplify / amplify-js

A declarative JavaScript library for application development using cloud services.
https://docs.amplify.aws/lib/q/platform/js
Apache License 2.0
9.44k stars 2.13k forks source link

Installing @aws-amplify/core adds node globals, which can make TS ignore potential errors #11736

Open bogdanailincaipnt opened 1 year ago

bogdanailincaipnt commented 1 year ago

Before opening, please confirm:

JavaScript Framework

Not applicable

Amplify APIs

Not applicable

Amplify Categories

Not applicable

Environment information

``` # Put output below this line System: OS: Linux 5.0 undefined CPU: (6) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz Memory: 0 Bytes / 0 Bytes Shell: 1.0 - /bin/jsh Binaries: Node: 16.20.0 - /usr/local/bin/node Yarn: 1.22.19 - /usr/local/bin/yarn npm: 9.4.2 - /usr/local/bin/npm pnpm: 8.6.10 - /usr/local/bin/pnpm npmPackages: @aws-amplify/core: ^5.8.0 => 5.8.0 @aws-amplify/core/internals/aws-client-utils: undefined () @aws-amplify/core/internals/aws-client-utils/composers: undefined () @aws-amplify/core/internals/aws-clients/pinpoint: undefined () typescript: 5.1.6 => 5.1.6 ```

Describe the bug

Installing @aws-amplify/core (or one of its dependends) adds NodeJS globals, which can make TS ignore potential errors.

The @aws-amplify/core package has the "@types/node-fetch" dependency in the "dependencies" list in its package.json. This is causing TS to think that NodeJS globals (eg. process) are available, even though we are developing for a browser environment.

Among NodeJS globals there are also methods like String.at, or Array.at, which are not supported in ES2020 for example. As you can see in the Stackblitz containers linked below, this can cause TS to miss some errors, since it has its target set as ES2020 in tsconfig.json.

Solution: "@types/node-fetch" and any other package related to types, react-native, or other devtools should be included in "devDependencies", instead of "dependencies" (explanation: https://docs.npmjs.com/cli/v9/configuring-npm/package-json#dependencies - "Please do not put test harnesses or transpilers or other "development" time tools in your dependencies object.", "If someone is planning on downloading and using your module in their program, then they probably don't want or need to download and build the external test or documentation framework that you use.").

I think this is important to solve, because there are a lot of users potentially affected by this. Vite for example sets its default target as ES2020 (roughly).

Expected behavior

TS should not be missing errors.

Reproduction steps

  1. Go to https://stackblitz.com/edit/vitejs-vite-krwz8t?file=package.json,src%2Fmain.ts&terminal=dev
  2. Run npx tsc in the terminal
  3. There should be 3 errors, but there are none.
  4. Go here, where @aws-amplify/core is not installed: https://stackblitz.com/edit/vitejs-vite-knsnqf?file=package.json,src%2Fmain.ts&terminal=dev
  5. Run npx tsc in the terminal
  6. You can see that 3 errors are found

You can also hover over process, at in the main.ts file to see how TS sees them (as NodeJS globals, or as missing).

Code Snippet

// Put your code below this line.

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

cwomack commented 1 year ago

Hello, @bogdanailincaipnt 👋 and thank you for opening this issue. We are currently looking into this and potentially addressing it with a fix soon. We'll keep you updated on the progress, but appreciate your detailed findings and reproduction steps!

cwomack commented 1 year ago

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.

bogdanailincaipnt commented 1 year ago

Nice! I tested with "@aws-amplify/core": "6.0.1-next.a1ea0f2.0" and it looks like it's fixed. You can check here. The TS errors are showing when running npx tsc, which is good.

cwomack commented 11 months ago

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.

bogdanailincaipnt commented 9 months ago

Hello again, sorry to say this, but I found that the issue still occurs when importing from 'aws-amplify'. You can see here: https://stackblitz.com/edit/vitejs-vite-ibxcgw?file=package.json,tsconfig.json,src%2Fmain.ts&terminal=dev.

If you comment out the line that does import 'aws-amplify';, 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.

My goal is to be able to specify where I want global types from @types/node to be available (eg. only in files that run in a node environment). It is not correct for them to be considered available in files that are run in the browser.

bogdanailincaipnt commented 9 months ago

It might be caused by one of these lines: https://github.com/aws-amplify/amplify-js/blob/b4735328013829a0404545e551f39fcd33985717/packages/tsconfig.base.json#L22 https://github.com/aws-amplify/amplify-js/blob/b4735328013829a0404545e551f39fcd33985717/packages/auth/tsconfig.json#L5

AllanZhengYP commented 9 months ago

Hi @bogdanailincaipnt

Thank you for providing the detailed Stackblitz reproduction. Techinically the tsconfig.base.json shouldn't impact the output types, they are only useful for the library maintainers. But I see the problem from the sandbox. We'll investigate it and keep you post on this thread.

ashika112 commented 2 months ago

@bogdanailincaipnt Hi, i started looking into this issue and all our reference in Amplify V6 for @type/node is as dev dependencies. To your comment here, i removed the dependencies there and did a tagged release to test it in stackedBlitz and I still see the issue. While I am looking more into it, i am not sure how having a dev dependency could affect this way.

ashika112 commented 2 months ago

Looking through fresh build locally with just aws-amplify as dependency, i dont see any @types/node as well

Screenshot 2024-08-22 at 12 35 29 PM

bogdanailincaipnt commented 2 months ago

Hello @ashika112, I appreciate you looking into this! I opened the project locally as well, I think I may have found the cause. After runningnpm install locally, open the node_modules folder and search inside it for this text: /// <reference types="node" />. You should find lots of occurences (the ones listed below). Having /// <reference types="node" /> anywhere inside the code is like having "node" inside tsconfig "types". See https://www.typescriptlang.org/docs/handbook/triple-slash-directives.html#-reference-types-.

59 results - 47 files

@aws-sdk/types/dist-types/blob/runtime-blob-types.node.d.ts:
  1: /// <reference types="node" />
  2: /// <reference types="node" />
  3  import { Readable } from "stream";

@smithy/core/dist-types/submodules/cbor/cbor-types.d.ts:
  1: /// <reference types="node" />
  2  export type CborItemType = undefined | boolean | number | bigint | [CborUnstructuredByteStringType, Uint64] | string | CborTagType;

@smithy/core/dist-types/ts3.4/submodules/cbor/cbor-types.d.ts:
  1: /// <reference types="node" />
  2  export type CborItemType = undefined | boolean | number | bigint | [

@smithy/credential-provider-imds/dist-types/remoteProvider/httpRequest.d.ts:
  1: /// <reference types="node" />
  2: /// <reference types="node" />
  3  import { Buffer } from "buffer";

@smithy/credential-provider-imds/dist-types/ts3.4/remoteProvider/httpRequest.d.ts:
  1: /// <reference types="node" />
  2  import { Buffer } from "buffer";

@smithy/eventstream-serde-node/dist-types/EventStreamMarshaller.d.ts:
  1: /// <reference types="node" />
  2  import { Decoder, Encoder, EventStreamMarshaller as IEventStreamMarshaller, Message } from "@smithy/types";

@smithy/eventstream-serde-node/dist-types/utils.d.ts:
  1: /// <reference types="node" />
  2  import { Readable } from "stream";

@smithy/eventstream-serde-node/dist-types/ts3.4/EventStreamMarshaller.d.ts:
  1: /// <reference types="node" />
  2  import { Decoder, Encoder, EventStreamMarshaller as IEventStreamMarshaller, Message } from "@smithy/types";

@smithy/eventstream-serde-node/dist-types/ts3.4/utils.d.ts:
  1: /// <reference types="node" />
  2  import { Readable } from "stream";

@smithy/node-http-handler/dist-types/node-http-handler.d.ts:
  1: /// <reference types="node" />
  2: /// <reference types="node" />
  3  import { HttpHandler, HttpRequest, HttpResponse } from "@smithy/protocol-http";

@smithy/node-http-handler/dist-types/node-http2-connection-manager.d.ts:
  1: /// <reference types="node" />
  2  import { RequestContext } from "@smithy/types";

@smithy/node-http-handler/dist-types/node-http2-connection-pool.d.ts:
  1: /// <reference types="node" />
  2  import { ConnectionPool } from "@smithy/types";

@smithy/node-http-handler/dist-types/readable.mock.d.ts:
  1: /// <reference types="node" />
  2: /// <reference types="node" />
  3  import { Readable, ReadableOptions } from "stream";

@smithy/node-http-handler/dist-types/write-request-body.d.ts:
  1: /// <reference types="node" />
  2: /// <reference types="node" />
  3  import { HttpRequest } from "@smithy/types";

@smithy/node-http-handler/dist-types/stream-collector/collector.d.ts:
  1: /// <reference types="node" />
  2: /// <reference types="node" />
  3  import { Writable } from "stream";

@smithy/node-http-handler/dist-types/stream-collector/readable.mock.d.ts:
  1: /// <reference types="node" />
  2: /// <reference types="node" />
  3  import { Readable, ReadableOptions } from "stream";

@smithy/node-http-handler/dist-types/ts3.4/node-http-handler.d.ts:
  1: /// <reference types="node" />
  2  import { HttpHandler, HttpRequest, HttpResponse } from "@smithy/protocol-http";

@smithy/node-http-handler/dist-types/ts3.4/node-http2-connection-manager.d.ts:
  1: /// <reference types="node" />
  2  import { RequestContext } from "@smithy/types";

@smithy/node-http-handler/dist-types/ts3.4/node-http2-connection-pool.d.ts:
  1: /// <reference types="node" />
  2  import { ConnectionPool } from "@smithy/types";

@smithy/node-http-handler/dist-types/ts3.4/readable.mock.d.ts:
  1: /// <reference types="node" />
  2  import { Readable, ReadableOptions } from "stream";

@smithy/node-http-handler/dist-types/ts3.4/write-request-body.d.ts:
  1: /// <reference types="node" />
  2  import { HttpRequest } from "@smithy/types";

@smithy/node-http-handler/dist-types/ts3.4/stream-collector/collector.d.ts:
  1: /// <reference types="node" />
  2  import { Writable } from "stream";

@smithy/node-http-handler/dist-types/ts3.4/stream-collector/readable.mock.d.ts:
  1: /// <reference types="node" />
  2  import { Readable, ReadableOptions } from "stream";

@smithy/types/dist-types/blob/blob-payload-input-types.d.ts:
  1: /// <reference types="node" />
  2: /// <reference types="node" />
  3  import { Readable } from "stream";

@smithy/types/dist-types/http/httpHandlerInitialization.d.ts:
  1: /// <reference types="node" />
  2: /// <reference types="node" />
  3  import type { Agent as hAgent, AgentOptions as hAgentOptions } from "http";

@smithy/types/dist-types/streaming-payload/streaming-blob-common-types.d.ts:
  1: /// <reference types="node" />
  2  import type { Readable } from "stream";

@smithy/types/dist-types/streaming-payload/streaming-blob-payload-input-types.d.ts:
  1: /// <reference types="node" />
  2: /// <reference types="node" />
  3  import type { Readable } from "stream";

@smithy/types/dist-types/streaming-payload/streaming-blob-payload-output-types.d.ts:
  1: /// <reference types="node" />
  2: /// <reference types="node" />
  3  import type { IncomingMessage } from "http";

@smithy/types/dist-types/transform/client-payload-blob-type-narrow.d.ts:
  1: /// <reference types="node" />
  2: /// <reference types="node" />
  3  import type { IncomingMessage } from "http";

@smithy/types/dist-types/ts3.4/blob/blob-payload-input-types.d.ts:
  1: /// <reference types="node" />
  2  import { Readable } from "stream";

@smithy/types/dist-types/ts3.4/http/httpHandlerInitialization.d.ts:
  1: /// <reference types="node" />
  2  import { Agent as hAgent, AgentOptions as hAgentOptions } from "http";

@smithy/types/dist-types/ts3.4/streaming-payload/streaming-blob-common-types.d.ts:
  1: /// <reference types="node" />
  2  import { Readable } from "stream";

@smithy/types/dist-types/ts3.4/streaming-payload/streaming-blob-payload-input-types.d.ts:
  1: /// <reference types="node" />
  2  import { Readable } from "stream";

@smithy/types/dist-types/ts3.4/streaming-payload/streaming-blob-payload-output-types.d.ts:
  1: /// <reference types="node" />
  2  import { IncomingMessage } from "http";

@smithy/types/dist-types/ts3.4/transform/client-payload-blob-type-narrow.d.ts:
  1: /// <reference types="node" />
  2  import { IncomingMessage } from "http";

@smithy/util-stream/dist-types/getAwsChunkedEncodingStream.d.ts:
  1: /// <reference types="node" />
  2  import { GetAwsChunkedEncodingStream } from "@smithy/types";

@smithy/util-stream/dist-types/splitStream.d.ts:
  1: /// <reference types="node" />
  2  import type { Readable } from "stream";

@smithy/util-stream/dist-types/ts3.4/getAwsChunkedEncodingStream.d.ts:
  1: /// <reference types="node" />
  2  import { GetAwsChunkedEncodingStream } from "@smithy/types";

@smithy/util-stream/dist-types/ts3.4/splitStream.d.ts:
  1: /// <reference types="node" />
  2  import { Readable } from "stream";

undici-types/content-type.d.ts:
  1: /// <reference types="node" />
  2  

undici-types/cookies.d.ts:
  1: /// <reference types="node" />
  2  

undici-types/fetch.d.ts:
  2  // and https://github.com/node-fetch/node-fetch/blob/914ce6be5ec67a8bab63d68510aabf07cb818b6d/index.d.ts (MIT license)
  3: /// <reference types="node" />
  4  

undici-types/file.d.ts:
  1  // Based on https://github.com/octet-stream/form-data/blob/2d0f0dc371517444ce1f22cdde13f51995d0953a/lib/File.ts (MIT)
  2: /// <reference types="node" />
  3  

undici-types/filereader.d.ts:
  1: /// <reference types="node" />
  2  

undici-types/formdata.d.ts:
  1  // Based on https://github.com/octet-stream/form-data/blob/2d0f0dc371517444ce1f22cdde13f51995d0953a/lib/FormData.ts (MIT)
  2: /// <reference types="node" />
  3  

undici-types/patch.d.ts:
  1: /// <reference types="node" />
  2  

undici-types/websocket.d.ts:
  1: /// <reference types="node" />
  2  
ashika112 commented 2 months ago

@bogdanailincaipnt Thanks for the quick response and appreciated the deep dive :) the listed packages in the screenshot are all from @aws-sdk packages (repo) which amplify takes dependency on. I think it needs to be addressed by them so we could stop seeing this i guess. I did find one instance in Amplify Predictions, I will try to remove that and test it (I think, it should have been tree-shaken already though)

ashika112 commented 2 months ago

@bogdanailincaipnt Confirming the Predictions is tree shaken out from aws-amplify umbrella package. I would recommend opening an issue with aws-sdk repo on the above. We have a hard dependency on @aws-sdk/types and that seems to be the core of this issue which brings in @types/node.

ashika112 commented 2 months ago

@bogdanailincaipnt can we close this issue in favor of the one opened in aws-sdk since there is not much we can do at our end?

bogdanailincaipnt commented 2 months ago

@ashika112 I think this issue should be closed only after the aws-amplify package has a new release that uses a version of @aws-sdk/types that has the issue fixed.

ashika112 commented 2 months ago

@bogdanailincaipnt Sorry you are right. Notice we have pinned dependencies on @aws-sdk/types. Will wait on reply on the other ticket. Thanks. Will mark it accordingly.