Skip to content

Conversation

@Murderlon
Copy link
Member

@Murderlon Murderlon commented Oct 3, 2022

Note
This branch will be merged into the new 1.x base branch

  • Start transition to TypeScript with ts-migrate
  • Install @types/* dependencies
  • Transition to ESM with cjstoesm
  • Fix all type errors, but do not implement types properly yet. We'll do this in a follow up PR
  • New .eslintrc.json
  • New .editorconfig
  • Format code with prettier
  • Ran npx sort-package.json
  • Switch to Corepack with Yarn
    • Corepack makes sure no one has to worry about having the right package manager installed
    • It manages the yarn version for us
    • Added a yarn plugin which automatically adds @types/* packages when needed

@Murderlon Murderlon requested a review from mitjap October 3, 2022 16:44
@mitjap
Copy link
Collaborator

mitjap commented Oct 3, 2022

Did you consider using ts-standard for formater and static code checker (https://standardjs.com/)?
https://standardjs.com/#why-should-i-use-javascript-standard-style

Should we also look at PNPM as package manager?
https://pnpm.io/motivation

@Murderlon
Copy link
Member Author

Murderlon commented Oct 3, 2022

Did you consider using ts-standard for formater and static code checker (https://standardjs.com/)?

I'm personally quite happy with how Prettier formats and how many file types it supports. I went for eslint to keep things minimal for now and not drown in warnings. If there are particular reasons you prefer standard over these two, I'm open to arguments.

Should we also look at PNPM as package manager?
https://pnpm.io/motivation

I was thinking of switching to corepack, which we also successfully use in Uppy, combined with Yarn V3 (with normal node_modules). Corepack ships with Node.js >= 14.20 (or can be installed globally) and lets all users install modules without installing a package manager. How do you feel about that?

Bonus points for yarn: auto install types if needed

* 1.x:
  0.8.1
  Fix validation of upload-length header in patch handler (#303)
  Bump @google-cloud/storage from 6.4.1 to 6.5.2 (#304)
  Bump aws-sdk from 2.1206.0 to 2.1227.0 (#305)
  Revert "Bump tus-js-client from 2.3.1 to 3.0.0" (#302)
  Emit upload complete event when using creation-with-upload extension (#301)
  GCS: fix error handling when checking for bucket existence (#299)
@mitjap
Copy link
Collaborator

mitjap commented Oct 3, 2022

I don't have any strong reasons to use standardjs over prettier and eslint. I like that it is single tool that does multiple things and is without configuration and has (at least for my taste) reasonable defaults.

@Murderlon
Copy link
Member Author

Murderlon commented Oct 4, 2022

I don't have any strong reasons to use standardjs over prettier and eslint. I like that it is single tool that does multiple things and is without configuration and has (at least for my taste) reasonable defaults.

I've been trying it locally in this branch with ts-standard and snazzy for the reporter. It's indeed nice to have these standards without thinking about it. It produces almost 800 errors at the moment, which is a lot but I have to go through all files anyway. The biggest problem I currently face is that I can't find a language server for ts-standard, only for standard, which means I can't get diagnostics in my editor (neovim) and that is kind of a deal breaker for me. Relying just on CLI output is tedious.

We could as an alternative use eslint-config-standard-with-typescript. It may defeat the purpose a bit of zero config, but we could continue to use Prettier (which does format more aggressively and more file types) and existing editor integrations.

Another alternative is trying xo?

What do you think?

@Murderlon
Copy link
Member Author

Murderlon commented Oct 4, 2022

Update: I implemented xo as an eslint plugin. I went with xo instead of standard because:

  • It's mostly similar to standard, but has a couple extra rules for best practices such as unoptimized regex's.
  • Almost everyone in the JS ecosystem has already eslint integration in their editor, so nothing new is required
  • AFAIK setting up the config is more a one time investment after which we will have the flexibility should we need it.
  • Current setup formats with eslint + prettier so there is no worry at all about manual formatting.

Note that I won't be fixing all the eslint errors in this PR but in a follow up PR which also does the types properly. Tests still run successfully locally and have been running successfully up until this last commit.

@Murderlon
Copy link
Member Author

Also switched to Corepack + Yarn V3. Setup caching for yarn dependencies in CI, and added the typescript yarn plugin. README as been updated for this.

That's it for this PR. Final call for comments :)

@Murderlon Murderlon added the 1.0 This is for the 1.0 major version label Oct 4, 2022
@mitjap
Copy link
Collaborator

mitjap commented Oct 5, 2022

Just a heads up. demo/server.js file is not migrated to *.ts

@Murderlon
Copy link
Member Author

Just a heads up. demo/server.js file is not migrated to *.ts

This is on purpose. I figured we want to keep the example demo in JS to make it as accessible as possible for everyone. It just references the build output now instead of the source.

@Murderlon Murderlon merged commit 0af8aca into 1.x Oct 5, 2022
@Murderlon Murderlon deleted the ts-migrate branch October 5, 2022 18:15
@Murderlon
Copy link
Member Author

Merged :)

Tackling proper types and eslint errors now. Properly best to hold off a bit more with big changes till the next PR is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1.0 This is for the 1.0 major version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants