[IOCOM-181] Migrate node 18 #141
Conversation
Garma00
requested review from
BurnedMarshal,
AleDore and
balanza
as code owners
Jira Pull Request LinkThis Pull Request refers to the following Jira issue IOCOM-181 |
src/index.ts
Outdated
| @@ -109,7 +109,8 @@ const withSpidAuthMiddleware = ( | |||
| res: express.Response, | |||
| next: express.NextFunction | |||
| ): void => { | |||
| passport.authenticate("spid", async (err, user) => { | |||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
| passport.authenticate("spid", async (err: any, user: any) => { | |||
There was a problem hiding this comment.
I think the types can be inserted here.
There was a problem hiding this comment.
Passport types are not that good. If you go and see all strategies it's a mess of any types for callbacks and fullfill functions. I would just not throw away too much time searching the types. The were inplicit any before, now they are explicit. There's not too much difference.
There was a problem hiding this comment.
We can use unknown instead of any. It's compatible with the current implementation and we don't have to disable the linter.
src/index.ts
Outdated
| @@ -109,7 +109,8 @@ const withSpidAuthMiddleware = ( | |||
| res: express.Response, | |||
| next: express.NextFunction | |||
| ): void => { | |||
| passport.authenticate("spid", async (err, user) => { | |||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
| passport.authenticate("spid", async (err: any, user: any) => { | |||
There was a problem hiding this comment.
Passport types are not that good. If you go and see all strategies it's a mess of any types for callbacks and fullfill functions. I would just not throw away too much time searching the types. The were inplicit any before, now they are explicit. There's not too much difference.
src/index.ts
Outdated
| @@ -109,7 +109,8 @@ const withSpidAuthMiddleware = ( | |||
| res: express.Response, | |||
| next: express.NextFunction | |||
| ): void => { | |||
| passport.authenticate("spid", async (err, user) => { | |||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
| passport.authenticate("spid", async (err: any, user: any) => { | |||
There was a problem hiding this comment.
We can use unknown instead of any. It's compatible with the current implementation and we don't have to disable the linter.
yarn.lock
Outdated
There was a problem hiding this comment.
We can upgrade the yarn.lock more selectively. The only required upgrade are:
typescript@^4.9.5
node-redis-mock@^0.2.14 // remove
mock-redis-client@^0.91.13 // remove
io-ts@2.2.20, io-ts@^2.2.16
io-ts-types@^0.5.16
version "0.5.19" // upgrade
fp-ts@2.13.1, fp-ts@^2.11.0
"@types/node@~18.13.0"
"@pagopa/ts-commons@^11.0.0"
There was a problem hiding this comment.
@BurnedMarshal During those migrations to Node 18 we are regenerating the yarn.lock in order to upgrade all the libraries, I would do the same here for consistency, what's your take?
There was a problem hiding this comment.
Ok. But it's a risky operation. Can we run locally this library (using the docker compose) and try to complete a login process?
Even delete this folder (unused now).
|
Kudos, SonarCloud Quality Gate passed!
|
List of Changes
Motivation and Context
End of life Node 18
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: