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

Fix directly calling operations on the frontend #1992

Merged
merged 33 commits into from
May 7, 2024
Merged

Conversation

sodic
Copy link
Contributor

@sodic sodic commented Apr 24, 2024

Fixes #1923.

I'll do all the testing once this gets approved.
I've already tested it a couple of times and it all works, but I'll hold of the final just-in-case tests until all changes are final.

Tasks:

  • Fix query API.
  • Figure out what to do with the type Query, Action, Operation, QueryFor, OperationFor. Should we expose some of them and remove duplication? - Issue made: Revisit types exported from the SDK #2020
  • Test queries with no payload.
  • Test queries with payload.
  • Test optimistic updates on queries with no payload.
  • Test optimistic updates on queries with payload.
  • Test caching on queries with payload.
  • Test caching on queries without payload.
  • Fix query api types.
  • Test actions.
  • Test todoApp build.
  • Update docs.
  • Test CRUD

@sodic sodic force-pushed the filip-fix-frontend-queries branch from 4e4cf5a to 83d24dc Compare April 25, 2024 13:00
Copy link
Contributor Author

@sodic sodic Apr 25, 2024

Choose a reason for hiding this comment

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

That's right. I've pulled a Miho.

In the future, we'll probably want to replace this with an existing type testing library:
https://stackoverflow.com/a/58831534

Copy link
Member

Choose a reason for hiding this comment

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

Open an issue for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already have one: #1951

) {
query.queryCacheKey = [relativeQueryPath]
// PRIVATE API (used in SDK)
export function buildAndRegisterQuery<Input, Output>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having a proper function (i.e., a smart constructor) seemed cleaner than having a function with side effects that you might forget to call.

// PRIVATE API (used in SDK)
export function buildAndRegisterQuery<Input, Output>(
queryFn: ClientOperation<Input, Output>,
{ queryCacheKey, queryRoute, entitiesUsed }:
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 took the creation of the queryCacheKey out to make it easily accessible in createQuery. The bonus is that this function now has fewer responsibilities.

Comment on lines 55 to 63
export type QueryFor<BackendQuery extends GenericBackendOperation> =
QueryForFunction<QueryFunctionFor<BackendQuery>>

type QueryFunctionFor<BackendQuery extends GenericBackendOperation> =
OperationRpcFor<BackendQuery>

// PRIVATE API (needed in SDK)
type QueryForFunction<QF extends QueryFunction<never, unknown>> =
QF & QueryMetadata
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 decided to avoid duplicating the same code for Queries and Actions this time around, mostly because that's what caused an incorrect type in RPC calls for queries.


// PRIVATE API (needed in SDK)
export type OperationRpcFor<BackendOperation extends GenericBackendOperation> =
Parameters<BackendOperation> extends []
Copy link
Contributor Author

@sodic sodic Apr 29, 2024

Choose a reason for hiding this comment

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

TS Playground link explaining why this check was necessary.

Comment on lines +46 to +47
const sleep = (ms: number) => new Promise((res) => setTimeout(res, ms))
await sleep(5000)
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'll comment this out after testing.

Comment on lines +22 to +39
async function logAll() {
const tasks = await getTasks()
console.info('Got tasks:', tasks)

const someId = tasks.map((task) => task.id).find((id) => id)
if (!someId) {
console.info('No tasks found')
} else {
const task = await getTask({ id: someId })
console.info(`Got task with id ${someId}`, task)
}

const date = await getDate()
console.info('Got date:', date)

const anything = await getAnything()
console.info('Got anything:', anything)
}
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'll comment this out after testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only formatting.

Comment on lines +62 to +68
export const getAnything: GetAnything = async () => {
return 'anything'
}

export const getTrueVoid = (async () => {
return 'anything'
}) satisfies GetTrueVoid
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 is all that's new. I've added these Queries to cover all edge cases with my types tests in TestRpcTypes.

The rest of the file is formatting.

@sodic sodic marked this pull request as ready for review April 29, 2024 12:56
Copy link
Contributor

@infomiho infomiho left a comment

Choose a reason for hiding this comment

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

Good stuff!

I like the cleanup you did, left some minor comments

type GetTask,
type GetTasks,
type GetDate,
GetAnything,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not prefix it with type as well?

@@ -43,8 +43,8 @@ export const updateTaskIsDone: UpdateTaskIsDone<
}

// Uncomment to test optimistic updates
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to comment this again or we want to keep it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I guess it's better if we keep it. It will make it obvious that optimistic updates are broken. Good call

Copy link
Contributor Author

@sodic sodic May 8, 2024

Choose a reason for hiding this comment

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

Left it in but shortened the delay: #2025

export function areThereAnyTasks(
tasks: Task[] | undefined
): tasks is NonEmptyArray<Task> {
return !!(tasks && tasks.length > 0)
}

const Todo = () => {
logAll()
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be a lot of work, but maybe output the values you received here and show it on the client somehow and then maybe create a headless test to check? Something like https://github.com/wasp-lang/wasp/pull/1995/files#diff-4ff0892633d41d0e5068ff93600cab7bf876e4b1bf2e1195e23836b513017fc5R1

Copy link
Contributor Author

@sodic sodic May 2, 2024

Choose a reason for hiding this comment

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

Good idea, but too much work for now (I was going to remove these logs).

I'll definitely create an issue for it though. Here it is: #2011


import './Main.css'
import { getName } from './user'
// Necessary to trigger type tests.
import './TestRpcTypes'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend maybe creating a type-tests folder or something like that and then import a index.ts from that folder to keep things a bit more organised.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then you could put the helpers in typeTestHelpers or something like that and then the RPC tests just in its own file.

// For the details of this specification, see
// https://github.com/wasp-lang/wasp/pull/1090#discussion_r1159732471

// This should be [],
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe put a TODO here

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'll do you one better: #2004

@@ -12,5 +12,5 @@ export const {= operationName =}: QueryFor<{= operationTypeName =}> = createQuer
)
{=/ queries =}

// PRIVATE API
export { addMetadataToQuery } from './core'
// PRIVATE API (used in SDK)
Copy link
Contributor

Choose a reason for hiding this comment

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

If used only in SDK, do you need to expose in the package.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that's what we'll do as part of #1922.

But for now, yes (since other parts of SDK use it as a package import).

query.queryCacheKey = [relativeQueryPath]
// PRIVATE API (used in SDK)
export function buildAndRegisterQuery<Input, Output>(
queryFn: ClientOperation<Input, Output>,
Copy link
Contributor

Choose a reason for hiding this comment

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

How not use QueryFunction here?

Copy link
Contributor Author

@sodic sodic May 2, 2024

Choose a reason for hiding this comment

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

Nice catch. I introduced QueryFunction after writing this code and forgot to update it.

{ queryCacheKey, queryRoute, entitiesUsed }:
{ queryCacheKey: string[], queryRoute: Route, entitiesUsed: string[] }
): QueryForFunction<typeof queryFn> {
const query = queryFn as QueryForFunction<typeof queryFn>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice cast

query.route = queryRoute
addResourcesUsedByQuery(query.queryCacheKey, entitiesUsed)

return query

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: extra new line

Comment on lines 55 to 63
export type QueryFor<BackendQuery extends GenericBackendOperation> =
QueryForFunction<QueryFunctionFor<BackendQuery>>

type QueryFunctionFor<BackendQuery extends GenericBackendOperation> =
OperationRpcFor<BackendQuery>

// PRIVATE API (needed in SDK)
type QueryForFunction<QF extends QueryFunction<never, unknown>> =
QF & QueryMetadata
Copy link
Contributor

Choose a reason for hiding this comment

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

I found this block hard to parse, maybe put some why comment on top to make it easier for us in the future to get back into this code.

Copy link
Contributor

@infomiho infomiho left a comment

Choose a reason for hiding this comment

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

LGTM, make sure that CI is passing successfully

return rqUseQuery({
queryKey,
queryFn: () => queryFn(queryKey, queryFnArgs),
queryKey: makeQueryCacheKey(query, queryFnArgs),
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 is unfortunate duplication.

Will explain soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Covered here: #2017

@sodic sodic merged commit 1699862 into main May 7, 2024
6 checks passed
@sodic sodic deleted the filip-fix-frontend-queries branch May 7, 2024 11:25
sodic added a commit that referenced this pull request May 8, 2024
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.

Rethink directly calling Operations from the client
3 participants