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

refc: rewrite the project in typescript #706

Merged
merged 27 commits into from Jul 12, 2022
Merged

Conversation

gamemaker1
Copy link
Contributor

@gamemaker1 gamemaker1 commented Jul 7, 2022

Related Issues

Description

This PR:

  • rewrites the library in Typescript
  • makes this package an ES module.
  • adds ESLint and Prettier
  • adds Husky and Lint Staged
  • adds a contributing guide
  • adds lint and test steps to the GitHub workflow
  • adds issue and pull request templates

Checklist

  • The issues that this PR fixes/closes have been mentioned above.
  • What this PR adds/changes/removes has been explained.
  • All tests (pnpm test) pass.
  • The linter (pnpm lint) does not throw an errors.
  • All added/modified code has been commented, and
    methods/classes/constants/types have been annotated with TSDoc comments.

@gamemaker1
Copy link
Contributor Author

I've tried to stick as close to the style guide as possible, but please let me know if there's something I missed and should change!

@leerob
Copy link
Member

leerob commented Jul 7, 2022

This is awesome! Going to dig into this and give it a thorough review.

@schemburkar
Copy link
Contributor

@gamemaker1 I have ported my work in #580 to code based on your current PR. How do you want me to push this? Directly to this PR or separate one?
cc: @leerob

@gamemaker1
Copy link
Contributor Author

@schemburkar That was really fast 🎉 Thanks for the amazing work on the PR!

I think it makes sense to keep the certificate PR separate and merge it in after this PR.

I want to make a couple of other PRs too, like the logging one and changes from useful forks like https://github.com/warren-bank/node-serve, but I'll wait until this one is merged and then make those separately. It'll make it easier for the Vercel team to review changes :]

config/husky/pre-commit Show resolved Hide resolved
source/main.ts Show resolved Hide resolved
source/types.ts Show resolved Hide resolved
source/utilities/config.ts Show resolved Hide resolved
@leerob
Copy link
Member

leerob commented Jul 10, 2022

Everything is working as expected 👍
image

package.json Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
tsconfig.json Show resolved Hide resolved
package.json Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
Copy link
Member

@mrmckeb mrmckeb left a comment

Choose a reason for hiding this comment

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

This looks great, a huge effort - thanks!

Let me know if you want help or want to pair on any of the comments, more than happy to help with any of it.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
source/utilities/server.ts Outdated Show resolved Hide resolved
source/utilities/server.ts Outdated Show resolved Hide resolved
source/utilities/server.ts Outdated Show resolved Hide resolved
source/utilities/server.ts Outdated Show resolved Hide resolved
source/utilities/server.ts Outdated Show resolved Hide resolved
source/utilities/cli.ts Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@gamemaker1
Copy link
Contributor Author

I have to step away for now, but once I am bacj I will address the ts-expect-error and eslint-disable comments as @mrmckeb pointed out. Thanks for the explanation and the hints @mrmckeb!

@gamemaker1
Copy link
Contributor Author

Removed all the ts-expect-error and eslint-disable comments that @mrmckeb pointed out. Thanks for pointing it out and helping me remove them!

@gamemaker1
Copy link
Contributor Author

Any other changes I should make?

@leerob leerob added the major label Jul 11, 2022
Copy link
Member

@leerob leerob left a comment

Choose a reason for hiding this comment

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

All of my comments have been addressed. To reiterate, we will be releasing this as a major version and can write a nice summary in the release notes to let folks know what changed.

Let's get two other +1s and we can ship it.

@gamemaker1
Copy link
Contributor Author

That's great!

Once this PR is merged, I would love to help out with adding the tests and some features from forks like https://github.com/warren-bank/node-serve; looking forward to collaborating with all of you on it :]

Copy link
Member

@mrmckeb mrmckeb left a comment

Choose a reason for hiding this comment

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

This is great. I'm not an expert on this package, but the changes you've made look logical and you've done a great job of typing and adhering to our style guide - thank you!

tsconfig.json Outdated Show resolved Hide resolved
@leerob leerob merged commit d2a5187 into vercel:main Jul 12, 2022
@leerob
Copy link
Member

leerob commented Jul 12, 2022

Thank you @gamemaker1 🙏 Would love to send you some Vercel swag (shoot me an email lee@vercel.com).

@leerob
Copy link
Member

leerob commented Jul 12, 2022

serve@14.0.0-canary.0 published to npm.

@leerob
Copy link
Member

leerob commented Jul 12, 2022

Confirmed things are working as expected, publishing stable – will keep an eye on issues 😄

@leerob
Copy link
Member

leerob commented Jul 12, 2022

So far I've only seen one small bug: #711

@gamemaker1
Copy link
Contributor Author

Would love to send you some Vercel swag (shoot me an email).

Wow, thank you! I will send you an email today :]

@gamemaker1 gamemaker1 deleted the refc/typescript branch July 14, 2022 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ESLint and move to TypeScript
6 participants