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

Typescript conversion #153

Open
wants to merge 64 commits into
base: master
Choose a base branch
from

Conversation

CapsAdmin
Copy link

@CapsAdmin CapsAdmin commented Oct 11, 2021

This converts the entire repo to typescript. The first few commits try to keep the spirit of the project but later on it gets more opinionated to suit Typescript.

It's far from perfect. It has many type errors and a relaxed tsconfig. Tests should run fine though.

Some major changes are:

  • Replace Zod with Joi as it makes more sense for Typescript.
  • Use Typegoose instead of mongoose directly, this library avoids typing code multiple times. (but at the cost of more complex syntax with decorators)
  • Use modern import/export syntax instead of require as it's needed for typesript.
  • Remove index files that import code and re-exports the code. This confuses auto import and find references.
  • Upgrade packages.

On a mid to high level I don't think this project has a good architecture for Typescript. To get the most out of Typescript, the code need to be structured in a more one-pass way rather than partially defined functions. I think this comes from the middleware pattern which makes more sense in javascript.

The middleware pattern is supported by typescript, but I personally don't think it's very typesafe.

For example, router.post('/register', validate(authValidation.register), authController.register); has the validation disconnected from the controller's register function from typescripts point of view.

One solution to this is to globally extend the Request type in express, but this makes the type available everywhere in all Request's.

Another more optimal way (IMO) is to avoid using middlewares and instead "plug in" the middleware so it becomes part of the control flow instead.

ie something like

export const registerSchema = z.object({
  body: z.object({
    email: z.string(),
    password: z.string(),
    name: z.string(),
  }),
});

router.post("user/register", async (req, res) => {
  const { email, password, name } = registerSchema.parse(req).body;

  const user = await userService.createUser({email, password, name});
  const tokens = await tokenService.generateAuthTokens(user);

  res.status(httpStatus.CREATED).send({ user, tokens });
});

The same goes for the mongoose plugins. There is no way for typescript to know that a schema will have a plugin unless you explicitly define the type after you've also called the plugin function.

So overall there will be a lot of double and tripple typing and telling typescript that some particular code has a certain type and some other code has the same type in order to make it typesafe while keeping the spirit of the project.

I tend to lean towards making things as typesafe as possible and make as few types as possible relying on inference. But I also recognize that for some this is not important or desirable.

2 tests fail when doing this because of how jest.spyOn expects an object, but is now getting a module.  I'm not sure how to elegantly solve this so I'll just leave it for now as it shouldn't become a problem once we go to typescript

there are also some hacks needed for jest to work with es6 modules
- jest has to be configured to read ts-node
- we can remove the "yarn jest" hack
- pm2 seems to have its own node_modules, so we have to install typescript and ts-node in postinstall
- all tests pass now
- had to upgrade jest version
- obviously the codebase is not typed so there are many errors, but it still runs as expected
this plays nicer with the language server's ability to find references and automatically import modules you want to use
also add default formatter and recommended extensions for vscode
@markvital
Copy link

Is there any work on this?
@hagopj13 Are there any plans for TypeScript support?

package.json Outdated Show resolved Hide resolved
@daveydee33
Copy link

I really like this. I would really like to use a Typescript version of this great boilerplate project :)

@CapsAdmin
Copy link
Author

I believe this is ready for a review. I haven't tested it that much apart from the test suite and there are some unresolved type errors left. Would be nice if someone else wants to do the rest.

One fundemental change I've done is to change auth and validate to be like this:

router
  .route('/:userId')
  .get(userController.getUser)
...
export const getUser = catchAsync(async (req, res) => {
  await auth(req, 'getUsers');
  const { params } = userValidation.getUser.parse(req);
  const user = await userService.getUserById(params.userId);
  if (!user) {
    throw new ApiError(httpStatus.NOT_FOUND, 'User not found');
  }
  res.send(user);
});

Rather than auth and validate being middleware. This allows us to more easily get the type of the request into our controller function.

auth also returns the user if sucesful, otherwise throws an UNAUTHORIZED ApiError.

I also think the pagination and toJSON mongoose plugins should be classes that are implemented or extended. Or maybe they can just be their own functions. (so paginate(user, ...) as opposed to user.paginate(...))

@CapsAdmin CapsAdmin marked this pull request as ready for review April 4, 2022 18:25
"express-rate-limit": "^6.3.0",
"helmet": "^5.0.2",
"http-status": "^1.5.0",
"joi": "^17.6.0",

Choose a reason for hiding this comment

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

I think you can remove this joi dependency if you replaced it with zod instead.

Copy link

@samihamine samihamine left a comment

Choose a reason for hiding this comment

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

I think that the work you have done here is GREAT, thank you very much for this contribution 🙏💐
I'll try to play with it in the following days and tell you if I find more errors/comments, and I hope by then that this pull request will be validated 😊

"@types/express-rate-limit": "^6.0.0",
"@types/jest": "^27.4.1",
"@types/jsonwebtoken": "^8.5.8",
"@types/mongoose": "^5.11.97",

Choose a reason for hiding this comment

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

I think you can also delete @types/mongoose package. Mongosse publishes its own, so no need external types

import { toJSON } from './plugins/toJSON.plugin';
import { paginate } from './plugins/paginate.plugin';
import { Role, roles } from '../config/roles';
import { prop, getModelForClass, pre, plugin, DocumentType } from '@typegoose/typegoose';
Copy link

Choose a reason for hiding this comment

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

In my opinion, I find that "Typegoose" brings more complexity and less flexibility to the code. Unless I'm mistaken I don't have the impression that the types work well at the level of services (for example with "User" in services/user.services) but this is surely due to a bad manipulation on my part.

@ghost
Copy link

ghost commented Oct 31, 2022

It would be nice if one could fix the hundreds of lint error. I've went through lots of them and it actually showed bugs here and there.
But i dont know what to do to fix the type warning with "id" being "any" and used as a string

@CapsAdmin
Copy link
Author

I don't have much motivation left to continue this, but overall I personally think it's best if the original author rethinks how this should be done with typescript from the ground up. I feel the overall structure resembles more of how you'd setup a Javascript project.

This might be fine for some Javascript developers wanting to try Typescript though.

@ghost
Copy link

ghost commented Oct 31, 2022

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

4 participants