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

meta: add support for TypeScript plugins #4640

Merged
merged 46 commits into from
Oct 17, 2023
Merged

meta: add support for TypeScript plugins #4640

merged 46 commits into from
Oct 17, 2023

Conversation

aduh95
Copy link
Member

@aduh95 aduh95 commented Aug 22, 2023

  • Adds support for TS sources in package.
  • Edits tsconfig.json so running yarn tsc generates the .d.ts files corresponding to the .ts.
  • Removes support for CJS sources in packages, which we no longer use since Uppy 3.0.
  • Upgrades TypeScript so we can use allowImportingTsExtensions
  • Adds yarn build:ts to check for TS errors.
  • Adds a script that migrate a JS package to TS

To migrate a package, run node private/js2ts/index.mjs @uppy/utils. This will rename all the files from .jsx to .tsx and .js to .ts, and change the import statements.

@socket-security
Copy link

socket-security bot commented Aug 22, 2023

New, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@babel/plugin-transform-typescript 7.22.15 None +0 130 kB nicolo-ribaudo
parcel 2.10.0...2.9.3, 2.8.3...2.9.3 None +105/-150 499 MB devongovett
tsd 0.22.0...0.28.1 None +1/-1 53.8 MB sindresorhus

🚮 Removed packages: execa@6.1.0, typescript@4.8.4

@arturi
Copy link
Contributor

arturi commented Aug 31, 2023

@Murderlon ready for your review / test, please 👌

@arturi arturi requested a review from Murderlon August 31, 2023 16:19
tsconfig.shared.json Outdated Show resolved Hide resolved
@Murderlon Murderlon self-assigned this Sep 5, 2023
@Murderlon
Copy link
Member

I was briefly able to testing typing a package, @uppy/utils, to see what it is like. Overall it seems to work well, so job well done! 👏

Some observations:

  • The private/js2ts/index.mjs script only works on Node.js 20.1.0. Ideally this should be able to run on LTS as well as I reckon most members and contributors use that too.
  • Types in tests are currently a problem. Because they are excluded in tsconfig, the settings don't apply there, resulting in errors such as An import path can only end with a '.ts' extension when 'allowImportingTsExtensions' is enabled, which is a setting we have in fact set. One solution might be to simply not convert tests to TypeScript, but that means importing from the compiled version and making tests dependent on a build step. Another options could be to have a dev tsconfig, which does include tests and it only used locally (using a different command in package.json which explicitly passes the other tsconfig).
  • We probably want to have @yarnpkg/plugin-typescript.

I wanted to investigate a bit more and contribute some of these changes but other things ended up taking more time this week. Next week I'm not working. Excusez-moi.

@aduh95
Copy link
Member Author

aduh95 commented Sep 8, 2023

  • The private/js2ts/index.mjs script only works on Node.js 20.1.0. Ideally this should be able to run on LTS as well as I reckon most members and contributors use that too.

AFAICT it works with Node.js 18.17+, which is LTS. Maybe you are using an older LTS version?

bin/build-ts.mjs Show resolved Hide resolved
e2e/start-companion-with-load-balancer.mjs Show resolved Hide resolved
private/js2ts/index.mjs Show resolved Hide resolved
Co-authored-by: Antoine du Hamel <antoine@transloadit.com>
private/js2ts/index.mjs Outdated Show resolved Hide resolved
@aduh95
Copy link
Member Author

aduh95 commented Oct 3, 2023

My editor resolves to the nearest tsconfig. When working with a package such as @uppy/utils this means that tsconfig is resolved and not the root one, which is where we added one for tests. Hence my LSP doesn't work correctly for tests.

Shouldn't you fix that / use one that works correctly? Or maybe disable it for test files if you can't find a quick workaround.

At the very least I would expect that the same tsconfig settings are applied to tests (either all of them or extend'd)

It seems to me we may want different settings (e.g. we do not want to transpile them to lib/). Extending is a good idea though.

@nickrttn
Copy link
Contributor

nickrttn commented Oct 3, 2023

Furthermore we should tests whether we even need a different tsconfig for tests. In unified we have 300 repositories with TypeScript and none of them separate tests into a different config (example).

Typically this is only required if you want to emit JavaScript files via tsc. Usually the tsconfig.json is picked up by language servers automatically so you want that to work for all files, then a tsconfig.build.json or something like that that excludes test files. That's how I set it up here: https://github.com/motifland/markprompt-js/tree/main/packages/react

@Murderlon
Copy link
Member

Murderlon commented Oct 3, 2023

Typically this is only required if you want to emit JavaScript files via tsc. Usually the tsconfig.json is picked up by language servers automatically so you want that to work for all files, then a tsconfig.build.json or something like that that excludes test files. That's how I set it up here: https://github.com/motifland/markprompt-js/tree/main/packages/react

Yes, this makes more sense! Currently we try to use both the root tsconfig and the local package tsconfig at the same time which isn't a good idea and won't work in all editors. With this suggestion we always resolve to locally scoped tsconfig, which has everything we need.

@aduh95 aduh95 requested a review from Murderlon October 3, 2023 12:13
bin/build-lib.js Outdated Show resolved Hide resolved
@Murderlon
Copy link
Member

Added my review in the other PR: #4699 (review)

@aduh95 aduh95 merged commit 773c8cb into main Oct 17, 2023
17 of 18 checks passed
@aduh95 aduh95 deleted the ts-packages-support branch October 17, 2023 13:38
Murderlon added a commit that referenced this pull request Oct 19, 2023
* main:
  meta: fix js2ts check
  meta: add support for TypeScript plugins (#4640)
@github-actions github-actions bot mentioned this pull request Oct 20, 2023
github-actions bot added a commit that referenced this pull request Oct 20, 2023
| Package                   | Version | Package                   | Version |
| ------------------------- | ------- | ------------------------- | ------- |
| @uppy/angular             |   0.6.1 | @uppy/progress-bar        |   3.0.4 |
| @uppy/audio               |   1.1.4 | @uppy/provider-views      |   3.6.0 |
| @uppy/aws-s3              |   3.4.0 | @uppy/react               |   3.1.4 |
| @uppy/aws-s3-multipart    |   3.8.0 | @uppy/remote-sources      |   1.1.0 |
| @uppy/box                 |   2.1.4 | @uppy/screen-capture      |   3.1.3 |
| @uppy/companion           |  4.10.0 | @uppy/status-bar          |   3.2.5 |
| @uppy/companion-client    |   3.5.0 | @uppy/store-default       |   3.0.5 |
| @uppy/compressor          |   1.0.5 | @uppy/store-redux         |   3.0.5 |
| @uppy/core                |   3.6.0 | @uppy/svelte              |   3.1.1 |
| @uppy/dashboard           |   3.6.0 | @uppy/thumbnail-generator |   3.0.6 |
| @uppy/drop-target         |   2.0.2 | @uppy/transloadit         |   3.3.2 |
| @uppy/dropbox             |   3.1.4 | @uppy/tus                 |   3.3.2 |
| @uppy/facebook            |   3.1.3 | @uppy/unsplash            |   3.2.3 |
| @uppy/file-input          |   3.0.4 | @uppy/url                 |   3.3.4 |
| @uppy/form                |   3.0.3 | @uppy/utils               |   5.5.2 |
| @uppy/golden-retriever    |   3.1.1 | @uppy/vue                 |   1.1.0 |
| @uppy/google-drive        |   3.3.0 | @uppy/webcam              |   3.3.4 |
| @uppy/image-editor        |   2.2.2 | @uppy/xhr-upload          |   3.4.2 |
| @uppy/informer            |   3.0.4 | @uppy/zoom                |   2.1.3 |
| @uppy/instagram           |   3.1.3 | uppy                      |  3.18.0 |
| @uppy/onedrive            |   3.1.4 |                           |         |

- @uppy/aws-s3-multipart: fix `TypeError` (Antoine du Hamel / #4748)
- meta: Bump tough-cookie from 4.1.2 to 4.1.3 (dependabot[bot] / #4750)
- meta: example: simplify code by using built-in `throwIfAborted` (Antoine du Hamel / #4749)
- @uppy/aws-s3-multipart: pass `signal` as separate arg for backward compat (Antoine du Hamel / #4746)
- meta: fix TS integration (Antoine du Hamel / #4741)
- meta: fix js2ts check (Antoine du Hamel)
- meta: add support for TypeScript plugins (Antoine du Hamel / #4640)
- @uppy/vue: export FileInput (mdxiaohu / #4736)
- meta: examples: update `server.py` (codehero7386 / #4732)
- @uppy/aws-s3-multipart: fix `uploadURL` when using `PUT` (Antoine du Hamel / #4701)
- @uppy/dashboard: auto discover and install plugins without target (Artur Paikin / #4343)
- meta: e2e: upgrade Cypress (Antoine du Hamel / #4731)
- @uppy/core: mark the package as side-effect free (Antoine du Hamel / #4730)
- meta: Bump postcss from 8.4.16 to 8.4.31 (dependabot[bot] / #4723)
- meta: test with the latest versions of Node.js (Antoine du Hamel / #4729)
- meta: e2e: update Parcel (Antoine du Hamel / #4726)
- meta: uppy: fix types (Antoine du Hamel / #4721)
- @uppy/core: type more events (Antoine du Hamel / #4719)
- @uppy/svelte: fix TS build command (Antoine du Hamel / #4720)
- @uppy/companion: Bucket fn also remote files (Mikael Finstad / #4693)
- @uppy/companion-client: fixup! Added Companion OAuth Key type (Murderlon / #4668)
- @uppy/companion-client: Added Companion OAuth Key type (Chris Pratt / #4668)
- meta: check for formatting in CI (Antoine du Hamel / #4714)
- meta: bump get-func-name from 2.0.0 to 2.0.2 (dependabot[bot] / #4709)
- meta: run Prettier on existing files (Antoine du Hamel / #4713)
@arturi arturi mentioned this pull request Oct 25, 2023
38 tasks
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.

None yet

5 participants