-
Notifications
You must be signed in to change notification settings - Fork 1
Mikec/visual examples #44
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
Conversation
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall! Good to deploy. A few things to clean up before merging, but no need for second review unless you want more eyes
example/.gitignore
Outdated
|
||
# Ignored for the template, you probably want to remove it: | ||
package-lock.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Ignored for the template, you probably want to remove it: | |
package-lock.json |
example/README.md
Outdated
npm install | ||
npm run build | ||
cd example | ||
npm install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
npm install | |
npm run build | |
cd example | |
npm install | |
npm run setup |
src/component/inspect.ts
Outdated
export const listTrees = query({ | ||
args: { | ||
take: v.optional(v.number()), | ||
}, | ||
returns: v.array( | ||
v.object({ | ||
root: v.id("btreeNode"), | ||
namespace: v.optional(v.any()), | ||
maxNodeSize: v.number(), | ||
_id: v.id("btree"), | ||
_creationTime: v.number(), | ||
}) | ||
), | ||
handler: async (ctx, args) => { | ||
const values = await ctx.db.query("btree").take(args.take ?? 100); | ||
return values; | ||
}, | ||
}); | ||
|
||
export const listTreeNodes = query({ | ||
args: { | ||
take: v.optional(v.number()), | ||
}, | ||
returns: v.array( | ||
v.object({ | ||
items: v.array(itemValidator), | ||
subtrees: v.array(v.id("btreeNode")), | ||
aggregate: v.optional(aggregate), | ||
_id: v.id("btreeNode"), | ||
_creationTime: v.number(), | ||
}) | ||
), | ||
handler: async (ctx, args) => { | ||
const values = await ctx.db.query("btreeNode").take(args.take ?? 100); | ||
return values; | ||
}, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: One risk here is that the returns
validator is tightly coupled to the component fields. We could use the ...schema.tables.btreeNode.validator.fields
trick. The convex-helpers
doc(schema, "btreeNode")
helper would also be handy here. We could alternatively not specify the return validator to not have folks rely on it tightly - it'd be v.any?
I had a thought the other day that the crud
helper could be really handy for components to avoid this boilerplate for basic get/list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ye I was running into strange typing errors when using ...schema.tables.btreeNode.validator.fields
ill try again
@@ -22,8 +22,8 @@ | |||
"setup": "npm i && npm run build && cd example && npm i && npx convex dev --once", | |||
"build:watch": "cd src && npx chokidar -d 1000 '../tsconfig.json' '**/*.ts' -c 'npm run build' --initial", | |||
"build": "npm run build:esm && npm run build:cjs", | |||
"build:esm": "tsc --project ./esm.json && echo '{\\n \"type\": \"module\"\\n}' > dist/esm/package.json", | |||
"build:cjs": "tsc --project ./commonjs.json && echo '{\\n \"type\": \"commonjs\"\\n}' > dist/commonjs/package.json", | |||
"build:esm": "tsc --project ./esm.json && node -e \"require('fs').writeFileSync('dist/esm/package.json', JSON.stringify({type:'module'}))\"", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we'll even need this package.json writing when we drop cjs
targets (we've done that already for other components)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷
import { internalMutation } from "./_generated/server"; | ||
import { v } from "convex/values"; | ||
|
||
export const resetAndSeed = internalMutation({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is being called from the hosted demo, and is doing a collect
on every table to delete them, then I'm worried it'll run into the limits. For synchronous, you could have it be an action that calls actions that do deletes in batches, or do it asynchronously, where it paginates each table recursively and seeds afterwards. Or each reset could return if they were "done" and if not, it could bail out and schedule itself until they're all fully deleted before seeding? the last one might be easiest - each .collect turning into a .take(1000), say
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ye fair enough, I did think about that but forgot to come back to it :P
example/convex/photos.ts
Outdated
const allPhotos = await ctx.db.query("photos").collect(); | ||
const albumNames = [...new Set(allPhotos.map((photo) => photo.album))]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI the .distinct on the streams helper would be useful here. then it could even be paginated, though that's not necessary here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was the original code from the example I think. Ye I think ill keep it simpler for now by not introducing the streams concept
example/convex/photos.ts
Outdated
}); | ||
|
||
/** | ||
* Get all available albums - useful for the UI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we drop the UI-oriented features lower than the illustrative ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep fair enough
@@ -78,6 +104,14 @@ export const pageOfPhotos = query({ | |||
}, | |||
returns: v.array(v.string()), | |||
handler: async (ctx, { offset, numItems, album }) => { | |||
// Check if the album has any photos first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if we don't do this? does it throw for offset of 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I think it did, maybe it shouldnt?
example/convex/shuffle.ts
Outdated
query, | ||
internalMutation, | ||
MutationCtx, | ||
} from "../../example/convex/_generated/server"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing a lot of ../../example/convex
that should just be ./
-- what's that about?
example/convex/shuffle.ts
Outdated
@@ -18,25 +23,26 @@ const randomize = new TableAggregate<{ | |||
sortKey: () => null, | |||
}); | |||
|
|||
const _addMusic = async (ctx: MutationCtx, { title }: { title: string }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const _addMusic = async (ctx: MutationCtx, { title }: { title: string }) => { | |
async function addMusicHandler(ctx: MutationCtx, { title }: { title: string }) { |
then you can define it below the addMusic
const, so it visually flows / args are right there.
In this PR I am adding a visual frontend to the example so users can go ahead and play around with it.
In a future PR (if all goes well with this one). I will get the example hosted so it can be linked to from this repo so people dont have to clone this repo to try out the examples.
This is quite a large PR where I have tried to confine everything to the example directory. I did however need to add a couple more queries to the "inspect" file in the root, this is so I can get more data to inspect the btrees.
I will be using these visualisations as the basis for a video on the Aggregate component that I can currently working on.