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.

Sign up for GitHub

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

Jump to bottom

strict null checks refactor #2390

Merged
merged 18 commits into from
Jan 4, 2023
Merged

strict null checks refactor #2390

merged 18 commits into from
Jan 4, 2023

Conversation

scopsy
Copy link
Contributor

@scopsy scopsy commented Dec 29, 2022 •
edited

What?

This pull request introduces the "strictNullCheck" of typescript. This feature adds additional type-checking to the compiler and helps to prevent null and undefined values from being assigned to variables.

Why?

  • To help us better throw API Errors when specific entities not found, as right now we have weire states when a not existing subscriberId was not found for example.
  • Explicitly defining null and undefined of interface members to help prevent mistakes from happening.

This PR mainly handles the needed refactoring for build to pass, further improved could be made and in some cases hacks been used to pass the build (for example usage of as).

@scopsy
ci: wip
ea13c2f 
ci: wip

Revert "ci: wip"

This reverts commit cc14cb5dab1ef25e94723ab5958b79e2561df875.

ci: wip
scopsy
scopsy commented Jan 1, 2023
View reviewed changes
apps/api/package.json
Comment on lines +21 to +22
"test": "cross-env TS_NODE_COMPILER_OPTIONS='{\"strictNullChecks\": false}' TZ=UTC NODE_ENV=test E2E_RUNNER=true mocha --timeout 10000 --require ts-node/register --exit --file e2e/setup.ts src/**/**/*.spec.ts",
"test:e2e": "cross-env TS_NODE_COMPILER_OPTIONS='{\"strictNullChecks\": false}' TZ=UTC NODE_ENV=test E2E_RUNNER=true mocha --timeout 10000 --require ts-node/register --exit --file e2e/setup.ts e2e/**/*.e2e.ts src/**/*.e2e.ts"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to add this since I only enabled strictNullCheck for API code and not tests for now (there is a lot of refactoring need to do there).

apps/api/src/app/events/services/mail-service/handlers/emailjs.handler.ts
@@ -10,12 +10,12 @@ export class EmailJsHandler extends BaseHandler {
}
buildProvider(credentials: ICredentials, from?: string) {
const config: IEmailJsConfig = {
from,
host: credentials.host,
from: from as string,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one will probably need a further refactoring for all the providers so we can specify what are the required fields per each provider. For now just used the as workaround

apps/api/src/app/events/usecases/process-subscriber/process-subscriber.usecase.ts
@@ -89,15 +91,18 @@ export class ProcessSubscriber {
})
);

const jobs: JobEntity[] = [];
const jobs: Omit<JobEntity, '_id'>[] = [];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the future probably better to refactor this into different types for creation and returned from dal. Will be using this workaround in some areas until then.

apps/api/src/app/events/usecases/process-subscriber/process-subscriber.usecase.ts Outdated Show resolved Hide resolved
packages/application-generic/src/factories/mail/handlers/mailgun.handler.ts
Comment on lines +18 to +21
apiKey: credentials.apiKey as string,
username: credentials.user as string,
domain: credentials.domain as string,
from: from as string,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like we have a duplication of all the provider factories on both API and application generic. didn't refactored it for now to avoid non strict null changes in this PR.

@scopsy scopsy marked this pull request as ready for review January 1, 2023 15:45
LetItRock
LetItRock approved these changes Jan 4, 2023
View reviewed changes
Copy link
Contributor

@LetItRock LetItRock left a comment

Choose a reason for hiding this comment

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

Thanks @scopsy!
You've caught and fixed so many bugs here! 🔥
We should merge this ASAP, because keeping it open will require a lot of effort

apps/api/src/app/auth/auth.module.ts Outdated Show resolved Hide resolved
apps/api/src/app/events/usecases/process-subscriber/process-subscriber.usecase.ts Outdated Show resolved Hide resolved
apps/api/tsconfig.json
@@ -1,6 +1,7 @@
{
"extends": "../../tsconfig.json",
"compilerOptions": {
"strictNullChecks": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, I think that we should have it defined in the root tsconfig.base.json, but I'm not sure if all packages use it... we should only redefine it in the tests like you did

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I first started as defining it for everything, unfortuently it was to much errors for me to handle. So I focused in this PR on the critical services. We will have to refactor it in the future to enable globally. Wdyt?

@scopsy scopsy merged commit e36c822 into next Jan 4, 2023
@scopsy scopsy deleted the strict-null-check branch January 4, 2023 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LetItRock LetItRock LetItRock approved these changes

@djabarovgeorge djabarovgeorge Awaiting requested review from djabarovgeorge

@p-fernandez p-fernandez Awaiting requested review from p-fernandez

Assignees
No one assigned
Labels
@novu/api @novu/dal @novu/shared @novu/web @novu/webhook @novu/ws providers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@scopsy @LetItRock