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

Enables strict null checks in SDK #2360

Merged
merged 29 commits into from
Nov 26, 2024
Merged

Enables strict null checks in SDK #2360

merged 29 commits into from
Nov 26, 2024

Conversation

infomiho
Copy link
Contributor

@infomiho infomiho commented Oct 28, 2024

This PR enables strictNullChecks option in the SDK tsconfig.json.

We want to enable this option to make sure Zod schemas are working properly before we start using them for env variables validation.

Left to do

  • Rough changes to get it to work
  • Go through changes one more time
    • Keep changes that are straightforward and long term
    • Add @ts-ignore and a TODO for things that need more work

Using @ts-ignore is not that problematic since it will unblock us for using Zod schemas, but also explicitly mark parts of the code base that need some work. This work was still needed before this PR, but it was implicit.

const { data: task, isLoading } = tasksCrud.get.useQuery({
id: parseInt(id, 10),
});
const { id } = useParams<{ id: 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.

Not related to this PR, but a quick fix since id is of type string | undefined.

@@ -1,9 +1,9 @@
{{={= =}=}}
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've started to work on enabling strict null checks while working on env variables, so these are some changes I left from that effort - env -> nodeEnv since I imagined that the import of env vars would look like import { env } from 'wasp/server'

@infomiho infomiho changed the title WIP: Enables strict null checks in SDK Enables strict null checks in SDK Oct 28, 2024
@infomiho infomiho requested a review from sodic October 28, 2024 15:04
@infomiho infomiho mentioned this pull request Oct 29, 2024
5 tasks
@infomiho infomiho force-pushed the miho-strict-null-checks branch from 18f9fd0 to a509762 Compare October 29, 2024 11:26
waspc/data/Generator/templates/sdk/wasp/auth/validation.ts Outdated Show resolved Hide resolved
waspc/data/Generator/templates/sdk/wasp/auth/user.ts Outdated Show resolved Hide resolved
waspc/data/Generator/templates/sdk/wasp/auth/useAuth.ts Outdated Show resolved Hide resolved
waspc/examples/todoApp/src/testTypes/operations/client.ts Outdated Show resolved Hide resolved
waspc/headless-test/examples/todoApp/src/auth/hooks.ts Outdated Show resolved Hide resolved
waspc/headless-test/examples/todoApp/src/server/actions.ts Outdated Show resolved Hide resolved
Comment on lines +13 to +14
username: process.env.SMTP_USERNAME!,
password: process.env.SMTP_PASSWORD!,
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is throwing all kinds of errors for me. I tried to build the server with TypeScript (npx tsc in .wasp/out/server) and it failed.

Does this work for you normally (I'm guessing it does, because you wouldn't know these types needed changing if it didn't).

I probably messed something up because it also fails on older versions of main. If everything's normal for you, I'll dig in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If npx tsc fails on server output on main, I'd say this is a matter for a different PR. Running it on main gives me:

...
Found 10 errors in 5 files.

Errors  Files
     1  src/auth/providers/config/discord.ts:13
     5  src/auth/providers/config/email.ts:34
     1  src/auth/providers/config/github.ts:13
     1  src/auth/providers/config/google.ts:13
     2  src/webSocket/initialization.ts:17

while running it on this branch gives me:

...
Found 18 errors in 8 files.

Errors  Files
     1  src/auth/providers/config/discord.ts:13
     5  src/auth/providers/config/email.ts:34
     1  src/auth/providers/config/github.ts:13
     1  src/auth/providers/config/google.ts:13
     2  src/auth/providers/email/login.ts:64
     3  src/auth/providers/email/resetPassword.ts:45
     3  src/auth/providers/email/signup.ts:160
     2  src/webSocket/initialization.ts:17

The new errors are fixed here: 630c107

I'd do it like this:

  • create a new issue to add npx tsc on the built server in the CI
  • create a new PR that add npx tsc on server in the CI
  • fix the remaining 10 errors in that PR

Copy link
Contributor

@sodic sodic left a comment

Choose a reason for hiding this comment

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

Nice! I'm approving.

Just make sure you cover these before merging:

I've fixed my part, left some comments explaining it, and gone through at your fixes.

I didn't run the e2e tests yet because you'll probably make a few more changes.

waspc/data/Generator/templates/sdk/wasp/auth/utils.ts Outdated Show resolved Hide resolved
Comment on lines +81 to 102

getJobScheduleData =
maybe
(object ["isDefined" .= False])
( \schedule ->
object
[ "isDefined" .= True,
"cron" .= J.cron schedule,
"args" .= getJobScheduleArgs (J.args schedule),
"options" .= getJobSchduleOptions (J.scheduleExecutorOptionsJson job)
]
)
getJobScheduleArgs =
maybe
(object ["isDefined" .= False])
(\args -> object ["isDefined" .= True, "json" .= Aeson.Text.encodeToLazyText args])

getJobSchduleOptions =
maybe
(object ["isDefined" .= False])
(\options -> object ["isDefined" .= True, "json" .= Aeson.Text.encodeToLazyText options])

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks great! Just please double check we didn't break anything (does the e2e test look alright to you).

We could add a few "complicated" job declarations to the job e2e test.

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've tested the scenarios:

  • having args, not having args
  • having options, not having options
  • having the root level schedule, not having schedule

The resulting code looks OK to me 👍

Copy link
Contributor

@sodic sodic left a comment

Choose a reason for hiding this comment

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

Crap, I forgot about this thing: #2360 (comment)

I have to revoke approval (sorry for the tease). We should either:

  • Fix all the errors and add the server build step to the CI (best option, but longest)
  • Not introduce new errors and create an issue for the old ones and building the server in the CI.

@infomiho
Copy link
Contributor Author

I've created an issue for adding npx tsc on server code in the CI: #2384

The new TS errors were removed in this commit: 630c107

Signed-off-by: Mihovil Ilakovac <mihovil@ilakovac.com>
@infomiho infomiho requested a review from sodic November 25, 2024 16:35
Copy link
Contributor

@sodic sodic left a comment

Choose a reason for hiding this comment

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

Nice work! :shipit:

@infomiho infomiho merged commit 75f0e80 into main Nov 26, 2024
6 checks passed
@infomiho infomiho deleted the miho-strict-null-checks branch November 26, 2024 14:35
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.

2 participants