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

Move to TypeScript #683

Draft
wants to merge 31 commits into
base: main
Choose a base branch
from
Draft

Move to TypeScript #683

wants to merge 31 commits into from

Conversation

Acconut
Copy link
Member

@Acconut Acconut commented Apr 16, 2024

This PR is an experiment to see how we can utilize TypeScript to transpile the source code into CommonJS/ESM code included in the package. The benefit is that we have type-checked source code and also don't have to manually maintain a type definition in index.d.ts.

Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should consider introducing a major version and only publish ESM for both Node.js and browser.

tsconfig.json Outdated
Comment on lines 5 to 12
"target": "ES2018",
"esModuleInterop": true,
"forceConsistentCasingInFileNames": true,
"strict": true,
"skipLibCheck": true,
"allowJs": true,
"checkJs": false,
"declaration": true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some recommended settings

Suggested change
"target": "ES2018",
"esModuleInterop": true,
"forceConsistentCasingInFileNames": true,
"strict": true,
"skipLibCheck": true,
"allowJs": true,
"checkJs": false,
"declaration": true,
"esModuleInterop": true,
"skipLibCheck": true,
"target": "es2022",
"allowJs": true,
"resolveJsonModule": true,
"moduleDetection": "force",
"isolatedModules": true,
"strict": true,
"noUncheckedIndexedAccess": true,
"sourceMap": true,
"declaration": true,

@Acconut
Copy link
Member Author

Acconut commented Apr 23, 2024

@Murderlon You mentioned that dual publishing a package containing CommonJS and ESM can lead to unexpected problems. Are there some links or resources digging more into this? Are you referring to what Node.js calls the dual package hazard? We have dual published this package for four years (since bdb6144) and didn't run into any problems and no major issues where reported to us.

By completely ditching CommonJS, I am worrying that we leave some application behind which are still using it and have a hard time upgrading to ESM (I think to recall that our website tus.io also still compiles to CommonJS, but I might get that wrong). Is this concern unjustified nowadays?

If so, it would be great if we could reduce the complexity of this package's setup.

@Murderlon
Copy link
Member

You mentioned that dual publishing a package containing CommonJS and ESM can lead to unexpected problems. Are there some links or resources digging more into this?

This from my experience + some other prominent maintainers I talk with. All my issues in the past with a package were from dual publishing, not ESM. Examples include Preact itself and even the package dual-publish, which is supposed to solve it.

It is possible and since we already have it working it's not really a problem. But at some point we should make the jump, ESM is the future.

By completely ditching CommonJS, I am worrying that we leave some application behind which are still using it and have a hard time upgrading to ESM

Technically everyone can use it, even in a CommonJS setup:

const tusPromise = import('tus-js-client')

module.exports = tusPromise.then((tus) => tus.default)

It's not the ideal developer experience, but it's important to note that tus-js-client is close to done for tus protocol v1. It's stable and it's totally fine to stay on the previous major version if you must for a while. And even without ditching CommonJS, this would be a major version anyway, and I think it's better to have less major versions (rather than major version for TS then later again for ESM).


Some tools which are importent to check:

@Murderlon
Copy link
Member

Some more warnings:
https://publint.dev/[email protected]

I mainly followed the recommendations from https://github.com/frehner/modern-guide-to-packaging-js-library with one exception: We don't use `browser` anymore, but consider the browser-versions the default. Node 12+ will automatically resolve to the node-version as it supports the new `exports` syntax.

`arethetypeswrong` and `publint` are also happy:

```
$ npx @arethetypeswrong/cli --pack .
tus-js-client v4.1.0

Build tools:
- typescript@^5.4.5

 No problems found 🌟

┌───────────────────┬─────────────────┬──────────────────────────────┐
│                   │ "tus-js-client" │ "tus-js-client/package.json" │
├───────────────────┼─────────────────┼──────────────────────────────┤
│ node10            │ 🟢              │ 🟢 (JSON)                    │
├───────────────────┼─────────────────┼──────────────────────────────┤
│ node16 (from CJS) │ 🟢 (CJS)        │ 🟢 (JSON)                    │
├───────────────────┼─────────────────┼──────────────────────────────┤
│ node16 (from ESM) │ 🟢 (ESM)        │ 🟢 (JSON)                    │
├───────────────────┼─────────────────┼──────────────────────────────┤
│ bundler           │ 🟢              │ 🟢 (JSON)                    │
└───────────────────┴─────────────────┴──────────────────────────────┘
$ npx publint
tus-js-client lint results:
All good!
```
@Acconut
Copy link
Member Author

Acconut commented May 28, 2024

I updated this PR to touch the export structure in package.json as little as possible and instead opened #693 to handle the restructuring of the exports in a separate PR.

* browser StreamSource TS

* TS upload and options

* Remove whitespace

* Fix type of default options

* Only pass strings to `HttpRequest#setHeader` (#709)

* setHeader value arg should be string

* Explicit casting of header values, number to string

* Fix syntax

* Fix lint

---------

Co-Authored-By: Marius Kleidl <[email protected]>

* Fix error emission

Co-Authored-By: Marius Kleidl <[email protected]>

* Allows `bytesTotal` to be null for progress events

Co-Authored-By: Marius Kleidl <[email protected]>

---------

Co-authored-by: Matthew Holloway <Matthew Holloway>
Co-authored-by: Marius Kleidl <[email protected]>
Co-authored-by: dragan_d_dragon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants