Convert to TypeScript #187

remcohaszing · 2024-09-06T10:51:33Z

This converts the code base from JavaScript to TypeScript. The tests are failing because of a reason I don’t understand. It’s complaining about the use of TypeScript’s CJS syntax.


 RUN  v2.0.5 /home/remco/Projects/transloadit/node-sdk

 ❯ test/integration/live-api.test.ts (0)
 ❯ test/unit/mock-http.test.ts (0)
 ❯ test/unit/test-pagination-stream.test.ts (0)
 ❯ test/unit/test-transloadit-client.test.ts (0)

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Failed Suites 4 ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

 FAIL  test/integration/live-api.test.ts [ test/integration/live-api.test.ts ]
 FAIL  test/unit/mock-http.test.ts [ test/unit/mock-http.test.ts ]
 FAIL  test/unit/test-pagination-stream.test.ts [ test/unit/test-pagination-stream.test.ts ]
 FAIL  test/unit/test-transloadit-client.test.ts [ test/unit/test-transloadit-client.test.ts ]
SyntaxError: Unexpected token '='
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/4]⎯

 Test Files  4 failed (4)
      Tests  no tests
   Start at  12:47:19
   Duration  389ms (transform 173ms, setup 0ms, collect 0ms, tests 0ms, environment 1ms, prepare 265ms)

This converts the use of `import =` and `export =` to ESM syntax. The ESM syntax is compiled to CJS.

remcohaszing · 2024-09-13T15:13:40Z

This is a breaking change as TransloaditClient was previously assigned as module.exports = TransloaditClient, but now it’s a named export.

Also rename `TransloaditClientOptions` to `Transloadit.Options`.

kvz · 2024-09-18T11:31:46Z

Do you want to wait until Mikael is back to give the final okay, or move ahead already?

remcohaszing · 2024-09-19T08:29:32Z

I think we can wait for Mikael to review this.

kvz · 2024-09-19T10:57:33Z

we can probably already set something up for auto-releases?

mifi

very nice stuff!

src/Transloadit.ts

tsconfig.build.json

mifi · 2024-09-28T18:29:17Z

tsconfig.json

+{
+  "exclude": ["src"],
+  "references": [{ "path": "./tsconfig.build.json" }],
+  "compilerOptions": {


I think we should "extends": ["@tsconfig/strictest"] as it adds some nice checks (or we can just pull out those rules from that file and put them in ours). however we could do that in a future PR.

I see some nice options there, but also some options I really dislike.

I could enable exactOptionalPropertyTypes, noPropertyAccessFromIndexSignature, and noUncheckedIndexedAccess, but all other options are more either undesired or more appropriate as lint rules.

I agree that some rules are better covered by eslint (because eslint is more flexible in that you can disable rules per-line, while typescript cannot). however I still think that:

isolatedModules I think it's recommended to enable this in order to enforce writing typescript in a style that is compatible with external tools like esbuild, tsx etc. tsx even recommends enabling it.

esModuleInterop: from my research it's generally recommended to set this to true, and tsx also recommends it.

skipLibCheck recommended by TypeScript to set to true

noImplicitOverride which eslint rule covers this?

noImplicitReturns eslint has consistent-return, however they recommend that we instead use the typescript rule.

what's the reason you really dislike these typescript rules?

isolatedModules is needed when compiling the TypeScript with tools other than tsc. vitest is also one such tools, so we should probably enable it indeed. Better would be to use verbatimModuleSyntax, but that’s not supported with Vitest apparently.

esModuleInterop is implied by "module": "node16". We don’t need to define it explicitly. Note that this option is risky for libraries (TypeScript generated invalid types if esModuleInterop is enabled microsoft/TypeScript#52779). The solution is to use verbatimModuleSyntax, which we can’t because of Vitest.

skipLibCheck is basically a huge @ts-ignore applied to all .d.ts files. This is useful if you have dependencies that have type errors. I prefer to disable this, as that helps identify possible compatibility issues with other dependencies. Whether or not to enable this, is subjective.

noImplicitOverride requires TypeScript specific syntax for overriding methods. I slightly prefer to not rely on that. In my experience it’s barely ever relevant though. I haven’t given it that much thought. I could see its value.

noImplicitReturns is a stylistic choice I just don’t like. I pretty strongly prefer the exact opposite style (unicorn/no-useless-undefined). I combine this with explicit type annotations (@typescript-eslint/explicit-function-return-type). That’s a good idea for libraries anyway, as it gives more control over the types you publish.

esModuleInterop is implied by "module": "node16".

ok, let's remove it.

skipLibCheck: false

I had problems with it before, where some library had type errors and i couldn't compile because of that, and i believe it's slower. but if it works with skipLibCheck: false now, then i guess why not. feel free to set it to false.

noImplicitOverride requires TypeScript specific syntax for overriding methods. I slightly prefer to not rely on that. In my experience it’s barely ever relevant though. I haven’t given it that much thought. I could see its value.

I also don't rely on this rule very often because i rarely use classes / oo & inheritance in javascript, but when i do, i think it has benefits because I can get an error if some library that I depend on change their signature of an inherited class prop without me knowing. and typescript already has a bunch of other non-js compatible syntax so i don't see any drawback with that.

noImplicitReturns is a stylistic choice I just don’t like. I pretty strongly prefer the exact opposite style (unicorn/no-useless-undefined). I combine this with explicit type annotations (@typescript-eslint/explicit-function-return-type). That’s a good idea for libraries anyway, as it gives more control over the types you publish.

Fel free to remove the rule. As for @typescript-eslint/explicit-function-return-type i'm not sure how to make it only apply to public functions, because i don't like having to manually type every internal function

I applied this feedback and also applied the new options to tsconfig.json.

I also removed noPropertyAccessFromIndexSignature, as it’s a stylistic choice our code currently doesn’t adhere to.

Instead of coercing a lot of checks into a single boolean, we now throw early.

This reverts commit 2e8072d.

mifi · 2024-10-05T09:36:09Z

did you run yarn test-integration locally? i cannot get it to succeed

for easier debugging

"exactOptionalPropertyTypes": true, "noPropertyAccessFromIndexSignature": true, "noUncheckedIndexedAccess": true,

mifi · 2024-10-05T11:50:22Z

found the bug in the tests

have also enabled the rules

    "exactOptionalPropertyTypes": true,
    "noPropertyAccessFromIndexSignature": true,
    "noUncheckedIndexedAccess": true,

mifi · 2024-10-05T11:52:00Z

this is odd. yarn prepack succeeds locally. edit: had to remove node_modules

"isolatedModules": true, "esModuleInterop": true, "skipLibCheck": true, "noImplicitOverride": true, "noImplicitReturns": true,

mifi · 2024-10-05T12:52:07Z

I've now also added the rules

    "isolatedModules": true,
    "esModuleInterop": true,
    "skipLibCheck": true,
    "noImplicitOverride": true,
    "noImplicitReturns": true,

but we can discuss it if you don't agree

mifi · 2024-10-05T13:00:29Z

I'm seeing a test failing locally though, it's very strange: should allow adding different types fails locally, but it happens also on main so i'm creating a new issue: #193 - maybe you can try also and see if you're seeing the same.

yarn test-integration is succeeding on the typescript branch: https://github.com/transloadit/node-sdk/actions/runs/11193428140/job/31118733305

remcohaszing · 2024-10-07T09:00:09Z

I also failed to run one integration test locally, but it does work on CI. That issue existed before I got involved. I have no idea what causes it.

Our code is not compatible with this.

remcohaszing · 2024-10-11T11:47:29Z

This broke the integration tests in the main branch, but I’m not sure why. Also why don’t we run those for pull requests?

mifi · 2024-10-20T18:46:11Z

It only failed for node22.3.0 https://github.com/transloadit/node-sdk/actions/runs/11290300021 so it might be a flake

We don't run in for all pull requests because

I'm afraid that cloudflare might block us or stop supporting github actions if we run it too often
they share the same transloadit account, and I'm not sure if running multiple test suites in parallell will work (or they affect eachother)

remcohaszing · 2024-10-21T08:52:20Z

The latest run seems to have succeeded again. So at least this PR didn’t cause breakage.

remcohaszing added 5 commits September 6, 2024 12:47

Convert to TypeScript

a6d48c5

Use named exports

cd59974

This converts the use of `import =` and `export =` to ESM syntax. The ESM syntax is compiled to CJS.

Fix linting issues

5dc7cc3

Fix ESLint configuration

3df58db

Fix Node.js 18 compatibility

18aa73a

Rename TransloaditClient to Transloadit

e955bbb

Also rename `TransloaditClientOptions` to `Transloadit.Options`.

remcohaszing requested a review from mifi September 19, 2024 08:29

mifi reviewed Sep 28, 2024

View reviewed changes

remcohaszing added 5 commits September 30, 2024 15:33

Split complex logical expression

2e8072d

Instead of coercing a lot of checks into a single boolean, we now throw early.

Merge branch 'main' into typescript

557f07d

Fix CI job name

e911c9b

Merge branch 'main' into typescript

cdfbc21

Revert "Split complex logical expression"

7ab50a4

This reverts commit 2e8072d.

remcohaszing requested a review from mifi October 3, 2024 11:19

mifi added 4 commits October 5, 2024 11:29

fix "as CreateAssemblyPromise"

da4d70d

fix "as { retryIn: number }"

7ee6abb

fix test server type

5abb090

prettier

667355f

mifi added 6 commits October 5, 2024 12:07

fix bug

22011e5

add timeout to tunnel

48f7a15

improve commend

d7409c9

improve check

abe0d9d

for easier debugging

enable ts rules

79b18eb

"exactOptionalPropertyTypes": true, "noPropertyAccessFromIndexSignature": true, "noUncheckedIndexedAccess": true,

run prettier

7f18d79

mifi added 2 commits October 5, 2024 14:32

fix ts error

702d103

add rules

b10c396

"isolatedModules": true, "esModuleInterop": true, "skipLibCheck": true, "noImplicitOverride": true, "noImplicitReturns": true,

mifi approved these changes Oct 5, 2024

View reviewed changes

remcohaszing added 2 commits October 11, 2024 11:49

Update TypeScript compiler options

366b6bd

Remove TypeScript option exactOptionalPropertyTypes

8140cbc

Our code is not compatible with this.

remcohaszing merged commit 3002ded into main Oct 11, 2024
8 checks passed

remcohaszing deleted the typescript branch October 11, 2024 09:57

remcohaszing mentioned this pull request Oct 21, 2024

Remove support for Node.js 16 #194

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert to TypeScript #187

Convert to TypeScript #187

remcohaszing commented Sep 6, 2024

remcohaszing commented Sep 13, 2024

kvz commented Sep 18, 2024

remcohaszing commented Sep 19, 2024

kvz commented Sep 19, 2024

mifi left a comment

mifi Sep 28, 2024

remcohaszing Sep 30, 2024

mifi Oct 5, 2024

remcohaszing Oct 7, 2024

mifi Oct 7, 2024

remcohaszing Oct 11, 2024

mifi commented Oct 5, 2024

mifi commented Oct 5, 2024

mifi commented Oct 5, 2024 •

edited

Loading

mifi commented Oct 5, 2024

mifi commented Oct 5, 2024 •

edited

Loading

remcohaszing commented Oct 7, 2024

remcohaszing commented Oct 11, 2024

mifi commented Oct 20, 2024

remcohaszing commented Oct 21, 2024

Convert to TypeScript #187

Convert to TypeScript #187

Conversation

remcohaszing commented Sep 6, 2024

remcohaszing commented Sep 13, 2024

kvz commented Sep 18, 2024

remcohaszing commented Sep 19, 2024

kvz commented Sep 19, 2024

mifi left a comment

Choose a reason for hiding this comment

mifi Sep 28, 2024

Choose a reason for hiding this comment

remcohaszing Sep 30, 2024

Choose a reason for hiding this comment

mifi Oct 5, 2024

Choose a reason for hiding this comment

remcohaszing Oct 7, 2024

Choose a reason for hiding this comment

mifi Oct 7, 2024

Choose a reason for hiding this comment

remcohaszing Oct 11, 2024

Choose a reason for hiding this comment

mifi commented Oct 5, 2024

mifi commented Oct 5, 2024

mifi commented Oct 5, 2024 • edited Loading

mifi commented Oct 5, 2024

mifi commented Oct 5, 2024 • edited Loading

remcohaszing commented Oct 7, 2024

remcohaszing commented Oct 11, 2024

mifi commented Oct 20, 2024

remcohaszing commented Oct 21, 2024

mifi commented Oct 5, 2024 •

edited

Loading

mifi commented Oct 5, 2024 •

edited

Loading