Update packages #156
Update packages #156
Conversation
| "inversify": "^2.0.1", | ||
| "joi": "^10.0.5", | ||
| "hapi": "^16.1.0", | ||
| "hapi-auth-jwt2": "^7.2.4", |
There was a problem hiding this comment.
Should probably be added in the Auth-related PR.
There was a problem hiding this comment.
True, these two slipped in by accident
There was a problem hiding this comment.
@dennari These two slipped back in
There was a problem hiding this comment.
Nevermind, I now remembered that this PR does, in fact, now have much more than just updated packages in it.
package.json
Outdated
| "jsonapi-serializer": "^3.4.2", | ||
| "jwks-rsa": "^1.1.1", |
There was a problem hiding this comment.
Is this also related to authentication?
src/json-api/json-api-module.ts
Outdated
| @@ -391,4 +393,8 @@ export class JsonApiModule { | |||
| }; | |||
| } | |||
|
|
|||
| public toApiNotificationConfiguration(configuration: NotificationConfiguration): ApiNotificationConfiguration { | |||
| return configuration as ApiNotificationConfiguration; | |||
There was a problem hiding this comment.
This function seems a bit suspect. The function name makes it seem that some sort of conversion is being done when, in reality, it's just hard-casting it to another type.
There was a problem hiding this comment.
It is a conversion. The fact that the interfaces are actually identical and the conversion can be done like this is an implementation detail that shouldn't concern the consumers of this method.
If they become unidentical at some point the conversion logic is placed here.
There was a problem hiding this comment.
Actually you're right in that the argument should be cloned at least
|
As mentioned, it might be a good idea to read over the CHANGELOG for every package that had a major or minor revision. Even though, technically, you should only be afraid of breaking changes for major version updates, I've noticed this not to always be the case. |
src/json-api/json-api-module.ts
Outdated
| @@ -394,7 +394,7 @@ export class JsonApiModule { | |||
| } | |||
|
|
|||
| public toApiNotificationConfiguration(configuration: NotificationConfiguration): ApiNotificationConfiguration { | |||
| return configuration as ApiNotificationConfiguration; | |||
| return { ...configuration } as ApiNotificationConfiguration; | |||
There was a problem hiding this comment.
If they have the same structure, shouldn't this work without the cast? Doesn't the as ApiNotificationConfiguration typecast hide errors in case the shapes change in the future?
There was a problem hiding this comment.
That's actually an excellent point. It should and it hides. This is something that's been unclear to me, i.e. these explicit vs. implicit casts in TypeScript. If I'm not mistaken, casting with as doesn't work in every situation and I don't really know what are the situations when it doesn't work. Here's some discussion about these matters: microsoft/TypeScript#7481.
|
Can this be deployed onto staging for testing purposes before it's merged into master? |
|
Yes, that's probably a good idea |
This reverts commit 01013e4.
|
Ok, checked the changes introduced in minor versions. LGTM. |
Don't require authentication in private/local endpoints
Also rename the custom email claim and improve gitlab client tests
Signup and team-tokens
Also change the AUTH_AUDIENCE default to localhost
dennari
force-pushed
the
update-packages
branch
from
6cf5a63 to
2603515
Compare
Allow users to fetch their teamtokens
This is quite a significant maintenance PR and should be subjected to major testing at least in staging.
NOTE: since
eventsourcewas updated, you should clean redis withbefore testing.