Skip to content

Conversation

clairep94
Copy link
Collaborator

@clairep94 clairep94 commented Sep 11, 2025

pr05 Typescript Migration 11: Migrate the server/routes folder

Should be reviewed after #3636

Changes:

  • simple update to ts for most files in the routes folder
    • Individual routes are expected to be reorganised in later PRs for clarity (eg. user routes file can be organised & annotated by their subdomain, like 'auth')
    • Attempted to migrate server.routes.js but this required Projects and User models to be migrated first

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

@clairep94 clairep94 changed the title Pr05/migrate routes folder pr05 Typescript Migration #11: Migrate server/routes folder Sep 11, 2025
@clairep94 clairep94 marked this pull request as ready for review September 15, 2025 20:01
@clairep94 clairep94 added the pr05 Grant Projects pr05 Grant Projects label Sep 15, 2025
@clairep94
Copy link
Collaborator Author

@raclim @khanniie ready for review after the server ts deps setup PR

khanniie
khanniie previously approved these changes Sep 21, 2025
Copy link
Collaborator

@khanniie khanniie left a comment

Choose a reason for hiding this comment

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

looks great, just a few small comments!!!

import isAuthenticated from '../utils/isAuthenticated';

const router = new Router();
export const router = Router();
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this export needed if you export default on it later?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed!

passport.authenticate(
service,
{ failureRedirect: '/login' },
(err: any, user: any) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

small nit but are there better types for these other than any? something from passport's types?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to find an error and user type from the passport @types, but I think they declare User as {}, and use err: any

I think when I migrate the User model, I can import it and use it, but error might just stay as any

@khanniie khanniie requested a review from raclim September 21, 2025 18:48
@clairep94
Copy link
Collaborator Author

@khanniie I updated to address some feedback, so might need a re-review when you're ready!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr05 Grant Projects pr05 Grant Projects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants