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

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions waspc/data/Generator/templates/sdk/wasp/api/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ window.addEventListener('storage', (event) => {
* standard format to be further used by the client. It is also assumed that given API
* error has been formatted as implemented by HttpError on the server.
*/
export function handleApiError(error: AxiosError<{ message?: string, data?: unknown }>): void {
export function handleApiError<T extends AxiosError<{ message?: string, data?: unknown }>>(error: T): T | WaspHttpError {
if (error?.response) {
// If error came from HTTP response, we capture most informative message
// and also add .statusCode information to it.
Expand All @@ -88,10 +88,10 @@ export function handleApiError(error: AxiosError<{ message?: string, data?: unkn
// That would require copying HttpError code to web-app also and using it here.
const responseJson = error.response?.data
const responseStatusCode = error.response.status
throw new WaspHttpError(responseStatusCode, responseJson?.message ?? error.message, responseJson)
return new WaspHttpError(responseStatusCode, responseJson?.message ?? error.message, responseJson)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we change this function?

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 we use handleApiError(error) which throws the error in some fn - TS thinks that fn has undefined as one of the return values.

If we do throw handleApiError(error) in the same fn, TS now knows this throws and then the fn doesn't return i.e. it doesn't return undefined.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, yeah, saw the other comment that addresses that. Makes sense.

I can't believe I'm saying this, but this is why monads are nice (don't tell Martin).

You can resolve this when you read it.

} else {
// If any other error, we just propagate it.
throw error
return error
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,6 @@ export async function login(data: { email: string; password: string }): Promise<
const response = await api.post('{= loginPath =}', data);
await initSession(response.data.sessionId);
} catch (e) {
handleApiError(e);
throw handleApiError(e);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ export async function requestPasswordReset(data: { email: string; }): Promise<{
const response = await api.post('{= requestPasswordResetPath =}', data);
return response.data;
} catch (e) {
handleApiError(e);
throw handleApiError(e);
}
}

Expand All @@ -17,6 +17,6 @@ export async function resetPassword(data: { token: string; password: string; }):
const response = await api.post('{= resetPasswordPath =}', data);
return response.data;
} catch (e) {
handleApiError(e);
throw handleApiError(e);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@ export async function signup(data: { email: string; password: string }): Promise
const response = await api.post('{= signupPath =}', data);
return response.data;
} catch (e) {
handleApiError(e);
throw handleApiError(e);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@ export async function verifyEmail(data: {
const response = await api.post('{= verifyEmailPath =}', data)
return response.data
} catch (e) {
handleApiError(e)
throw handleApiError(e)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ function AdditionalFormFields({
}: {
hookForm: UseFormReturn<LoginSignupFormFields>;
formState: FormState;
additionalSignupFields: AdditionalSignupFields;
additionalSignupFields?: AdditionalSignupFields;
}) {
const {
register,
Expand All @@ -302,7 +302,7 @@ function AdditionalFormFields({
disabled={isLoading}
/>
{errors[field.name] && (
<FormError>{errors[field.name].message}</FormError>
<FormError>{errors[field.name]!.message}</FormError>
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'm not sure why this is needed since we have the check errors[field.name] && (?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because, theoretically, the errors object could have a getter (a computed property) implemented like this:

get someFieldName () {
    return Math.random() > 0.5 ? undefined : { message: "You're lucky this time" }
}

In more precise terms, object field access is a function call and can easily be impure.

To get around this, you can define a local fieldError variable above the JSX and then use that.

Copy link
Contributor

@sodic sodic Nov 24, 2024

Choose a reason for hiding this comment

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

Ok, fun fact, my reasoning was wrong. TypeScript will happily allow this error to happen. Check it out.

Looks like TS specifically doesn't like if you do it twice (access a field of one object to get a string for accessing the field of another object). But my suggestion above still works. See here.

)}
</FormItemGroup>
);
Expand Down Expand Up @@ -341,7 +341,7 @@ function isFieldRenderFn(
}

function areAdditionalFieldsRenderFn(
additionalSignupFields: AdditionalSignupFields
additionalSignupFields?: AdditionalSignupFields
): additionalSignupFields is AdditionalSignupFieldRenderFn {
return typeof additionalSignupFields === 'function'
}
2 changes: 1 addition & 1 deletion waspc/data/Generator/templates/sdk/wasp/auth/login.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@ export default async function login(username: string, password: string): Promise

await initSession(response.data.sessionId)
} catch (error) {
handleApiError(error)
throw handleApiError(error)
}
}
4 changes: 2 additions & 2 deletions waspc/data/Generator/templates/sdk/wasp/auth/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { type AuthUserData } from '../server/auth/user.js';

import { auth } from "./lucia.js";
import type { Session } from "lucia";
import { throwInvalidCredentialsError } from "./utils.js";
import { createInvalidCredentialsError } from "./utils.js";

import { prisma } from 'wasp/server';
import { createAuthUserData } from "../server/auth/user.js";
Expand Down Expand Up @@ -66,7 +66,7 @@ async function getAuthUserData(userId: {= userEntityUpper =}['id']): Promise<Aut
})

if (!user) {
throwInvalidCredentialsError()
throw createInvalidCredentialsError()
}

return createAuthUserData(user);
Expand Down
2 changes: 1 addition & 1 deletion waspc/data/Generator/templates/sdk/wasp/auth/signup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@ export default async function signup(userFields: { username: string; password: s
try {
await api.post('{= signupPath =}', userFields)
} catch (error) {
handleApiError(error)
throw handleApiError(error)
}
}
2 changes: 1 addition & 1 deletion waspc/data/Generator/templates/sdk/wasp/auth/useAuth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ function createUserGetter(): Query<void, AuthUser | null> {
if (error.response?.status === 401) {
return null
} else {
handleApiError(error)
throw handleApiError(error)
}
}
}
Expand Down
6 changes: 4 additions & 2 deletions waspc/data/Generator/templates/sdk/wasp/auth/user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type {
AuthUser,
UserEntityWithAuth,
} from '../server/auth/user.js'
import { isNotNull } from '../universal/predicates.js'

/**
* We split the user.ts code into two files to avoid some server-only
Expand Down Expand Up @@ -35,6 +36,7 @@ export type { AuthUserData, AuthUser } from '../server/auth/user.js'
// PRIVATE API (used in SDK and server)
export function makeAuthUserIfPossible(user: null): null
export function makeAuthUserIfPossible(user: AuthUserData): AuthUser
export function makeAuthUserIfPossible(user: AuthUserData | null): AuthUser | null
export function makeAuthUserIfPossible(
user: AuthUserData | null,
): AuthUser | null {
Expand All @@ -45,13 +47,13 @@ function makeAuthUser(data: AuthUserData): AuthUser {
return {
...data,
getFirstProviderUserId: () => {
const identities = Object.values(data.identities).filter(Boolean);
const identities = Object.values(data.identities).filter(isNotNull);
return identities.length > 0 ? identities[0].id : null;
},
};
}

function findUserIdentity(user: UserEntityWithAuth, providerName: ProviderName): UserEntityWithAuth['auth']['identities'][number] | null {
function findUserIdentity(user: UserEntityWithAuth, providerName: ProviderName): NonNullable<UserEntityWithAuth['auth']>['identities'][number] | null {
if (!user.auth) {
return null;
}
Expand Down
61 changes: 43 additions & 18 deletions waspc/data/Generator/templates/sdk/wasp/auth/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ export async function updateAuthIdentityProviderData<PN extends ProviderName>(
): Promise<{= authIdentityEntityUpper =}> {
// We are doing the sanitization here only on updates to avoid
// hashing the password multiple times.
const sanitizedProviderDataUpdates = await sanitizeProviderData(providerDataUpdates);
const sanitizedProviderDataUpdates = await ensurePasswordIsHashed(providerDataUpdates);
const newProviderData = {
...existingProviderData,
...sanitizedProviderDataUpdates,
Expand All @@ -124,25 +124,39 @@ export async function updateAuthIdentityProviderData<PN extends ProviderName>(
});
}

type FindAuthWithUserResult = {= authEntityUpper =} & {
// PRIVATE API
export type FindAuthWithUserResult = {= authEntityUpper =} & {
{= userFieldOnAuthEntityName =}: {= userEntityUpper =}
}

// PRIVATE API
export async function findAuthWithUserBy(
where: Prisma.{= authEntityUpper =}WhereInput
): Promise<FindAuthWithUserResult> {
return prisma.{= authEntityLower =}.findFirst({ where, include: { {= userFieldOnAuthEntityName =}: true }});
): Promise<FindAuthWithUserResult | null> {
const result = await prisma.{= authEntityLower =}.findFirst({ where, include: { {= userFieldOnAuthEntityName =}: true }});

if (result === null) {
return null;
}

if (result.user === null) {
return null;
}

return result as FindAuthWithUserResult;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think preferrable options are:

const result = await prisma.auth.findFirst({ where, include: { user: true } });
return result && result.user && { ...result, user: result.user };

Or:

const result = await prisma.auth.findFirst({ where, include: { user: true } });

if (!result) {
  return null;
}

const { user, ...auth } = result;
return user && { user, ...auth };

I prefer the second one.

}

// PUBLIC API
export type CreateUserResult = {= userEntityUpper =} & {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also a new addition it seems. We should:

auth: {= authEntityUpper =} | null
}

// PUBLIC API
export async function createUser(
providerId: ProviderId,
serializedProviderData?: string,
userFields?: PossibleUserFields,
): Promise<{= userEntityUpper =} & {
auth: {= authEntityUpper =}
}> {
): Promise<CreateUserResult> {
return prisma.{= userEntityLower =}.create({
data: {
// Using any here to prevent type errors when userFields are not
Expand Down Expand Up @@ -261,34 +275,45 @@ export async function validateAndGetUserFields(
}

// PUBLIC API
export function deserializeAndSanitizeProviderData<PN extends ProviderName>(
export function getProviderData<PN extends ProviderName>(
Copy link
Contributor

Choose a reason for hiding this comment

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

This was a breaking change, right? We should update the changelog and the public API table.

providerData: string,
): Omit<PossibleProviderData[PN], 'hashedPassword'> {
return sanitizeProviderData(getProviderDataWithPassword(providerData));
}

// PUBLIC API
export function getProviderDataWithPassword<PN extends ProviderName>(
providerData: string,
{ shouldRemovePasswordField = false }: { shouldRemovePasswordField?: boolean } = {},
): PossibleProviderData[PN] {
// NOTE: We are letting JSON.parse throw an error if the providerData is not valid JSON.
let data = JSON.parse(providerData) as PossibleProviderData[PN];
return JSON.parse(providerData) as PossibleProviderData[PN];
Copy link
Contributor

Choose a reason for hiding this comment

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

You might not need the as here. I think the function's return type already narrows unknown to PossibleProviderData[PN].

}

if (providerDataHasPasswordField(data) && shouldRemovePasswordField) {
delete data.hashedPassword;
function sanitizeProviderData<PN extends ProviderName>(
providerData: PossibleProviderData[PN],
): Omit<PossibleProviderData[PN], 'hashedPassword'> {
if (providerDataHasPasswordField(providerData)) {
const { hashedPassword, ...rest } = providerData;
return rest;
} else {
return providerData;
}

return data;
}

// PUBLIC API
export async function sanitizeAndSerializeProviderData<PN extends ProviderName>(
providerData: PossibleProviderData[PN],
): Promise<string> {
return serializeProviderData(
await sanitizeProviderData(providerData)
await ensurePasswordIsHashed(providerData)
);
}

function serializeProviderData<PN extends ProviderName>(providerData: PossibleProviderData[PN]): string {
return JSON.stringify(providerData);
}

async function sanitizeProviderData<PN extends ProviderName>(
async function ensurePasswordIsHashed<PN extends ProviderName>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we make an issue for fixing this in the end?

The field name for hashedPassword doesn't allow for a correct type signature for this function and it can therefore be called multiple times?

If I got it right, we should rename password to hashedPassword where appropriate, create a new type, rename this function, and change it's type to be A -> B instead of A -> A?

providerData: PossibleProviderData[PN],
): Promise<PossibleProviderData[PN]> {
const data = {
Expand All @@ -309,6 +334,6 @@ function providerDataHasPasswordField(
}

// PRIVATE API
export function throwInvalidCredentialsError(message?: string): void {
throw new HttpError(401, 'Invalid credentials', { message })
export function createInvalidCredentialsError(message?: string): HttpError {
return new HttpError(401, 'Invalid credentials', { message })
sodic marked this conversation as resolved.
Show resolved Hide resolved
}
12 changes: 6 additions & 6 deletions waspc/data/Generator/templates/sdk/wasp/auth/validation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,37 +6,37 @@ const EMAIL_FIELD = 'email';
const TOKEN_FIELD = 'token';

// PUBLIC API
export function ensureValidEmail(args: unknown): void {
export function ensureValidEmail(args: object): void {
validate(args, [
{ validates: EMAIL_FIELD, message: 'email must be present', validator: email => !!email },
{ validates: EMAIL_FIELD, message: 'email must be a valid email', validator: email => isValidEmail(email) },
]);
}

// PUBLIC API
export function ensureValidUsername(args: unknown): void {
export function ensureValidUsername(args: object): void {
validate(args, [
{ validates: USERNAME_FIELD, message: 'username must be present', validator: username => !!username }
]);
}

// PUBLIC API
export function ensurePasswordIsPresent(args: unknown): void {
export function ensurePasswordIsPresent(args: object): void {
validate(args, [
{ validates: PASSWORD_FIELD, message: 'password must be present', validator: password => !!password },
]);
}

// PUBLIC API
export function ensureValidPassword(args: unknown): void {
export function ensureValidPassword(args: object): void {
validate(args, [
{ validates: PASSWORD_FIELD, message: 'password must be at least 8 characters', validator: password => isMinLength(password, 8) },
{ validates: PASSWORD_FIELD, message: 'password must contain a number', validator: password => containsNumber(password) },
]);
}

// PUBLIC API
export function ensureTokenIsPresent(args: unknown): void {
export function ensureTokenIsPresent(args: object): void {
validate(args, [
{ validates: TOKEN_FIELD, message: 'token must be present', validator: token => !!token },
]);
Expand All @@ -47,7 +47,7 @@ export function throwValidationError(message: string): void {
throw new HttpError(422, 'Validation failed', { message })
}

function validate(args: unknown, validators: { validates: string, message: string, validator: (value: unknown) => boolean }[]): void {
function validate(args: object, validators: { validates: string, message: string, validator: (value: unknown) => boolean }[]): void {
for (const { validates, message, validator } of validators) {
if (!validator(args[validates])) {
throwValidationError(message);
Expand Down
2 changes: 1 addition & 1 deletion waspc/data/Generator/templates/sdk/wasp/client/config.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{{={= =}=}}
import { stripTrailingSlash } from 'wasp/universal/url'
import { stripTrailingSlash } from '../universal/url.js'

const apiUrl = stripTrailingSlash(import.meta.env.REACT_APP_API_URL) || '{= defaultServerUrl =}';

Expand Down
18 changes: 11 additions & 7 deletions waspc/data/Generator/templates/sdk/wasp/client/operations/hooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,12 @@ export function useQuery<Input, Output>(
return rqUseQuery({
// todo: The full queryCacheKey is constructed in two places, both here and
// inside the Query. See https://github.com/wasp-lang/wasp/issues/2017
queryKey: makeQueryCacheKey(query, queryFnArgs),
queryFn: () => query(queryFnArgs),
// FIXME: query fns don't handle the `undefined` case correctly
// https://github.com/wasp-lang/wasp/issues/2017
queryKey: makeQueryCacheKey(query, (queryFnArgs as Input)),
// FIXME: query fns don't handle the `undefined` case correctly
// https://github.com/wasp-lang/wasp/issues/2017
queryFn: () => query(queryFnArgs as Input),
Comment on lines +31 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

Added the details into the issue: #2017 (comment)

...options,
})
}
Expand Down Expand Up @@ -138,7 +142,7 @@ type InternalOptimisticUpdateDefinition<ActionInput, CachedData> = {
* the current state of the cache and returns the desired (new) state of the
* cache.
*/
type SpecificUpdateQuery<CachedData> = (oldData: CachedData) => CachedData;
type SpecificUpdateQuery<CachedData> = (oldData: CachedData | undefined) => CachedData;
Copy link
Contributor

Choose a reason for hiding this comment

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

Also covered in #2017 (comment)


/**
* A specific, "instantiated" optimistic update definition which contains a
Expand Down Expand Up @@ -170,7 +174,7 @@ function translateToInternalDefinition<Item, CachedData>(
): InternalOptimisticUpdateDefinition<Item, CachedData> {
const { getQuerySpecifier, updateQuery } = publicOptimisticUpdateDefinition;

const definitionErrors = [];
const definitionErrors: string[] = [];
if (typeof getQuerySpecifier !== "function") {
definitionErrors.push("`getQuerySpecifier` is not a function.");
}
Expand Down Expand Up @@ -204,7 +208,7 @@ function makeOptimisticUpdateMutationFn<Input, Output, CachedData>(
CachedData
>[]
): typeof actionFn {
return (function performActionWithOptimisticUpdates(item?: Input) {
return (function performActionWithOptimisticUpdates(item: Input) {
const specificOptimisticUpdateDefinitions = optimisticUpdateDefinitions.map(
(generalDefinition) =>
getOptimisticUpdateDefinitionForSpecificItem(generalDefinition, item)
Expand Down Expand Up @@ -259,10 +263,10 @@ function makeRqOptimisticUpdateOptions<ActionInput, CachedData>(
);

// We're using a Map to correctly serialize query keys that contain objects.
const previousData = new Map();
const previousData: Map<QueryKey, CachedData | undefined> = new Map();
Copy link
Contributor

Choose a reason for hiding this comment

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

Tl;dr everything about this part of the code is covered in #2017 (comment), but it turns out this was actually correct (but would be nicer if we handled it more explicitly).

specificOptimisticUpdateDefinitions.forEach(({ queryKey, updateQuery }) => {
// Snapshot the currently cached value.
const previousDataForQuery: CachedData =
const previousDataForQuery: CachedData | undefined =
queryClient.getQueryData(queryKey);

// Attempt to optimistically update the cache using the new value.
Expand Down
Loading
Loading