arcjet / arcjet-js

Arcjet JS SDKs. Rate limiting, bot protection, email verification & attack defense for Node.js, Next.js, Bun & SvelteKit.
https://arcjet.com
Apache License 2.0
234 stars 5 forks source link

Builds fail when using `verbatimModuleSyntax` in TS compilation #1314

Closed yharaskrik closed 1 month ago

yharaskrik commented 1 month ago

I just installed the Arcjet SDK to use in our Nest application as a global guard but when I run the TS compilation with the verbatimModuleSyntax flag I get

node_modules/@arcjet/protocol/client.ts(1,10): error TS1484: 'Transport' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
node_modules/@arcjet/protocol/client.ts(10,3): error TS1484: 'ArcjetContext' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
node_modules/@arcjet/protocol/client.ts(12,3): error TS1484: 'ArcjetRequestDetails' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
node_modules/@arcjet/protocol/client.ts(13,3): error TS1484: 'ArcjetRule' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
node_modules/@arcjet/protocol/client.ts(91,55): error TS2339: Property 'entries' does not exist on type 'Headers'.
node_modules/@arcjet/protocol/client.ts(147,55): error TS2339: Property 'entries' does not exist on type 'Headers'.
node_modules/@arcjet/protocol/convert.ts(22,3): error TS1484: 'ArcjetRule' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
node_modules/@arcjet/protocol/convert.ts(23,3): error TS1484: 'ArcjetRateLimitRule' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
node_modules/@arcjet/protocol/convert.ts(24,3): error TS1484: 'ArcjetBotRule' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
node_modules/@arcjet/protocol/convert.ts(25,3): error TS1484: 'ArcjetEmailRule' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
node_modules/@arcjet/protocol/convert.ts(26,3): error TS1484: 'ArcjetTokenBucketRateLimitRule' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
node_modules/@arcjet/protocol/convert.ts(27,3): error TS1484: 'ArcjetFixedWindowRateLimitRule' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
node_modules/@arcjet/protocol/convert.ts(28,3): error TS1484: 'ArcjetSlidingWindowRateLimitRule' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
node_modules/@arcjet/protocol/convert.ts(30,3): error TS1484: 'ArcjetShieldRule' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
node_modules/@arcjet/protocol/convert.ts(60,13): error TS6133: '_exhaustive' is declared but its value is never read.
node_modules/@arcjet/protocol/convert.ts(79,13): error TS6133: '_exhaustive' is declared but its value is never read.
node_modules/@arcjet/protocol/convert.ts(100,13): error TS6133: '_exhaustive' is declared but its value is never read.
node_modules/@arcjet/protocol/convert.ts(121,13): error TS6133: '_exhaustive' is declared but its value is never read.
node_modules/@arcjet/protocol/convert.ts(144,13): error TS6133: '_exhaustive' is declared but its value is never read.
node_modules/@arcjet/protocol/convert.ts(161,13): error TS6133: '_exhaustive' is declared but its value is never read.
node_modules/@arcjet/protocol/convert.ts(178,13): error TS6133: '_exhaustive' is declared but its value is never read.
node_modules/@arcjet/protocol/convert.ts(199,13): error TS6133: '_exhaustive' is declared but its value is never read.
node_modules/@arcjet/protocol/convert.ts(218,13): error TS6133: '_exhaustive' is declared but its value is never read.
node_modules/@arcjet/protocol/convert.ts(239,13): error TS6133: '_exhaustive' is declared but its value is never read.
node_modules/@arcjet/protocol/convert.ts(301,13): error TS6133: '_exhaustive' is declared but its value is never read.
node_modules/@arcjet/protocol/convert.ts(532,13): error TS6133: '_exhaustive' is declared but its value is never read.
node_modules/@arcjet/protocol/index.ts(112,3): error TS4114: This member must have an 'override' modifier because it overrides a member in the base class 'ArcjetReason'.
node_modules/@arcjet/protocol/index.ts(138,3): error TS4114: This member must have an 'override' modifier because it overrides a member in the base class 'ArcjetReason'.
node_modules/@arcjet/protocol/index.ts(173,3): error TS4114: This member must have an 'override' modifier because it overrides a member in the base class 'ArcjetReason'.
node_modules/@arcjet/protocol/index.ts(177,3): error TS4114: This member must have an 'override' modifier because it overrides a member in the base class 'ArcjetReason'.
node_modules/@arcjet/protocol/index.ts(189,3): error TS4114: This member must have an 'override' modifier because it overrides a member in the base class 'ArcjetReason'.
node_modules/@arcjet/protocol/index.ts(204,3): error TS4114: This member must have an 'override' modifier because it overrides a member in the base class 'ArcjetReason'.
error: "tsc" exited with code 2

It looks like TS is not able to find the .js/.d.ts files and instead reverting to building from source? I am importing arcjet just like the examples

import arcjet, { detectBot, slidingWindow } from '@arcjet/node';

So it should just work, has anyone else seen this happen?

here is the output from using --explainFiles, I have never seen source like this get included in a build.

node_modules/@connectrpc/connect/dist/cjs/index.d.ts
  Imported via "@connectrpc/connect" from file 'node_modules/@arcjet/protocol/client.ts' with packageId '@connectrpc/connect/dist/cjs/index.d.ts@1.4.0'
node_modules/typeid-js/dist/index.d.ts
  Imported via "typeid-js" from file 'node_modules/@arcjet/protocol/index.ts' with packageId 'typeid-js/dist/index.d.ts@1.0.0'
node_modules/@arcjet/protocol/proto/decide/v1alpha1/decide_pb.d.ts
  Imported via "./proto/decide/v1alpha1/decide_pb.js" from file 'node_modules/@arcjet/protocol/index.ts' with packageId '@arcjet/protocol/proto/decide/v1alpha1/decide_pb.d.ts@1.0.0-alpha.21'
  Imported via "./proto/decide/v1alpha1/decide_pb.js" from file 'node_modules/@arcjet/protocol/convert.ts' with packageId '@arcjet/protocol/proto/decide/v1alpha1/decide_pb.d.ts@1.0.0-alpha.21'
  Imported via "./decide_pb.js" from file 'node_modules/@arcjet/protocol/proto/decide/v1alpha1/decide_connect.d.ts' with packageId '@arcjet/protocol/proto/decide/v1alpha1/decide_pb.d.ts@1.0.0-alpha.21'
  Imported via "./proto/decide/v1alpha1/decide_pb.js" from file 'node_modules/@arcjet/protocol/client.ts' with packageId '@arcjet/protocol/proto/decide/v1alpha1/decide_pb.d.ts@1.0.0-alpha.21'
node_modules/@arcjet/protocol/index.ts
  Imported via "./index" from file 'node_modules/@arcjet/protocol/convert.ts' with packageId '@arcjet/protocol/index.ts@1.0.0-alpha.21'
  Imported via "./index.js" from file 'node_modules/@arcjet/protocol/client.ts' with packageId '@arcjet/protocol/index.ts@1.0.0-alpha.21'
node_modules/@arcjet/protocol/convert.ts
  Imported via "./convert.js" from file 'node_modules/@arcjet/protocol/client.ts' with packageId '@arcjet/protocol/convert.ts@1.0.0-alpha.21'
node_modules/@arcjet/protocol/proto/decide/v1alpha1/decide_connect.d.ts
  Imported via "./proto/decide/v1alpha1/decide_connect.js" from file 'node_modules/@arcjet/protocol/client.ts' with packageId '@arcjet/protocol/proto/decide/v1alpha1/decide_connect.d.ts@1.0.0-alpha.21'
node_modules/@arcjet/protocol/client.ts
  Imported via "@arcjet/protocol/client.js" from file 'node_modules/arcjet/index.d.ts' with packageId '@arcjet/protocol/client.ts@1.0.0-alpha.21'
  Imported via "@arcjet/protocol/client.js" from file 'node_modules/@arcjet/node/index.d.ts' with packageId '@arcjet/protocol/client.ts@1.0.0-alpha.21'
node_modules/arcjet/index.d.ts
  Imported via "arcjet" from file 'node_modules/@arcjet/node/index.d.ts' with packageId 'arcjet/index.d.ts@1.0.0-alpha.21'
  Imported via "arcjet" from file 'node_modules/@arcjet/node/index.d.ts' with packageId 'arcjet/index.d.ts@1.0.0-alpha.21'
node_modules/@arcjet/node/index.d.ts
  Imported via '@arcjet/node' from file 'apps/aurora/src/app/arcjet.guard.ts' with packageId '@arcjet/node/index.d.ts@1.0.0-alpha.21'

This looks like the key part, the arcjet/protocol, index.ts is being included in the build from a JS file.

node_modules/@arcjet/protocol/client.ts
  Imported via "@arcjet/protocol/client.js" from file 'node_modules/arcjet/index.d.ts' with packageId '@arcjet/protocol/client.ts@1.0.0-alpha.21'
  Imported via "@arcjet/protocol/client.js" from file 'node_modules/@arcjet/node/index.d.ts' with packageId '@arcjet/protocol/client.ts@1.0.0-alpha.21'
davidmytton commented 1 month ago

Thanks for reporting @yharaskrik - we'll investigate and get back to you.

yharaskrik commented 1 month ago

Thanks for reporting @yharaskrik - we'll investigate and get back to you.

Thank you! I appreciate it, this is the command I am running.

bun run node_modules/typescript/bin/tsc --project apps/aurora/tsconfig.app.json --noEmit --explainFiles > tmp/explain

Tsconfig

{
    "extends": "./tsconfig.json",
    "compilerOptions": {
        "outDir": "../../dist/out-tsc",
        "types": ["node"],
        "emitDecoratorMetadata": true,
        "target": "esnext",
        "verbatimModuleSyntax": true
    },
    "exclude": ["src/**/*.spec.ts", "src/**/*.test.ts"],
    "include": ["src/**/*.ts"]
}

Base tsconfig


        "noImplicitReturns": true,
        "noImplicitOverride": true,
        "forceConsistentCasingInFileNames": true,
        "noFallthroughCasesInSwitch": true,
        "noUnusedLocals": true,
        "downlevelIteration": true,
        "sourceMap": true,
        "allowJs": true,
        "declaration": false,
        "moduleResolution": "node",
        "allowSyntheticDefaultImports": true,
        "emitDecoratorMetadata": true,
        "experimentalDecorators": true,
        "importHelpers": true,
        "target": "es2015",
        "esModuleInterop": true,
        "module": "esnext",
        "typeRoots": ["node_modules/@types"],
        "lib": ["es2017", "dom", "es2019", "es2020.Promise", "esnext", "webworker"],
        "skipLibCheck": true,
        "skipDefaultLibCheck": true,
        "resolveJsonModule": true,
        "useDefineForClassFields": false,
        "baseUrl": ".",
yharaskrik commented 1 month ago

Is there a reason the source files are shipped with the package and not just the .js and .d.ts?

blaine-arcjet commented 1 month ago

Is there a reason the source files are shipped with the package and not just the .js and .d.ts?

Mostly to make debugging easier. I think we still need to enable the "ts sourcemaps" though.

It's unclear to me with your provided setup is processing TypeScript files in node_modules, as that is not the default for TypeScript files. I wonder if the issue is bun itself, which I think prefers the .ts extension over others.

yharaskrik commented 1 month ago

Is there a reason the source files are shipped with the package and not just the .js and .d.ts?

Mostly to make debugging easier. I think we still need to enable the "ts sourcemaps" though.

It's unclear to me with your provided setup is processing TypeScript files in node_modules, as that is not the default for TypeScript files. I wonder if the issue is bun itself, which I think prefers the .ts extension over others.

I am only using bun as the runner (instead of node) and calling tsc. So it shouldn't have anything to do with how tsc is resolving the files but I can try without it. I am sure I'll get the same result.

blaine-arcjet commented 1 month ago

I can try without it.

Yes, please try it without. 🙏 The tsc compiler does not traverse node_modules unless explicitly specified in the include configuration, so we shouldn't see it try to compile the .ts files.

blaine-arcjet commented 1 month ago

I'm okay with updating our TypeScript source for verbatimModuleSyntax since it only seems like we're marking types explicitly as type.

However, your base config also sets "noUnusedLocals": true and "noImplicitOverride": true which I don't think we're going to support. We leverage https://www.typescriptlang.org/docs/handbook/2/narrowing.html#exhaustiveness-checking to check our switch statements which means the .ts files contain the extra variable while the .js output does not.

yharaskrik commented 1 month ago

Ok i figured it out, it was not a bun issue, and isn't an issue for a modern version of tsc, I have a secondary version of typescript installed (for legacy reasons I am trying to remove). Looks like the version of tsc in the node_modules/.bin folder was the old one not the new one, when I run with

bun run node_modules/typescript/bin/tsc --project apps/aurora/tsconfig.app.json --noEmit

or

node --max-old-space-size=8192 node_modules/typescript/bin/tsc --project apps/aurora/tsconfig.app.json --noEmit

It does not happen.

You are absolutely right that it should not be traversing node_modules.

This can now be closed!

yharaskrik commented 1 month ago

Note: looks like its back, our CI just failed with it (even though it passed locally earlier). I will keep trying to dig into it as it absolutely should not be doing this, super weird, but I want to use arcjet.

Not shipping the source files would fix it, but that change shouldn't be needed (I suspect none of our other packages are shipping source files and thats why we haven't run into this before)