-
Notifications
You must be signed in to change notification settings - Fork 61
[Package] PowerSync Nuxt Module #797
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
base: main
Are you sure you want to change the base?
Conversation
… and initial setup
…Supabase Todo List demo
|
|
Not sure what the chanset should include when releasing a new package |
stevensJourney
left a comment
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.
Just sharing some initial comments for some items which stood out from an initial review.
| └── nuxt.config.ts # Nuxt configuration | ||
| ``` | ||
|
|
||
| ## Learn More |
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.
It would be nice if this demo had a local Supabase config and quickstart for local development - which can be used to start the demo in only a few commands - like the YJS Demo has here.
| @@ -0,0 +1,27 @@ | |||
| import { column, Schema, Table } from '@powersync/web' | |||
|
|
|||
| export const local_bucket_data = new Table( | |||
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: We typically name these kinds of constants in upper case.
| /** | ||
| * Record fields from downloaded data, then build a schema from it. | ||
| */ | ||
| export class DynamicSchemaManager { |
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 looks like a copy from tools/diagnostics-app? It would be nice if we had this functionality in some shared location.
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.
As per our discussion, it's not a 100% copy past since I had to make some parts reactive, but there is a proposale that would make all this code disappear powersync-ja/powersync-sqlite-core#155
| // broadcastLogs: true, // need to be enabled for multitab support | ||
| // } | ||
|
|
||
| // @ts-expect-error - type error because we are forcing the vfs to be the OPFSCoopSyncVFS |
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 seems fishy. Why do we need to make this force assertion?
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 will be removed.
| // override settings to disable multitab as we can't use it right now | ||
| // options.flags = { | ||
| // ...options.flags, | ||
| // enableMultiTabs: true, // to support multitabe we need to write our won worker implementation |
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 you're overriding the PowerSyncDatabase, it might be possible (with some minor modifications) to actually implement your own web worker implementation, using the current primitives, which contains the required sync hooks.
| import type { DynamicSchemaManager } from './DynamicSchemaManager' | ||
| import type { Ref } from 'vue' | ||
| /** | ||
| * Tracks per-byte and per-operation progress for the Rust client. |
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.
Just sharing this again. I'd really prefer we did not duplicate these implementations.
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.
Addressed above
packages/nuxt/src/runtime/composables/usePowerSyncInspectorDiagnostics.ts
Outdated
Show resolved
Hide resolved
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.
Not entirely sure what the benefit of a dedicated script is over using npx npkill or even rm -rf demos/**/node_modules
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 quickly vibe-coded this to reset the entire monorepo, not only the demos, which allowed me to iterate faster when I was having dependency and build issues. It can be improved to delete the pnpm-lock file and other unnecessary folders like .nuxt inside the nuxt demo folder.
I don't mind removing it completely either, but I found it useful tbh, that's why I pushed it.
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 we do plan on keeping this script, can we consider adding to the package.json https://github.com/powersync-ja/powersync-js/blob/main/package.json#L20-L21 ?
…ashboard link for Nuxt Supabase Todo List demo
…, README, and inspector layout
…e nuxt.config.ts accordingly
…move unused JSON type definitions from module.ts and usePowerSyncInspectorDiagnostics.ts
…osable usage examples
…nd add YAML schema reference in sync-rules.yaml
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 not sold on bundling the Kysely integration with the Nuxt package, if users want to use another ORM they would have to pay the price of both ORMs' bundles.
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 agree with this. I will see how to optionally include the dependency and composable via a config flag.
| @@ -0,0 +1,58 @@ | |||
| <template> | |||
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'd suggest that we keep a consistent orders for templates/script blocks across all our components in this demo.
| return navigateTo('/') | ||
| } | ||
| }, | ||
| { immediate: true }, |
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.
A few of these watches that happen to be immediate:true, could be swapped by watchEffect() - https://vuejs.org/guide/essentials/watchers#watcheffect
| import type { Database, TaskRecord } from '~/powersync/AppSchema' | ||
|
|
||
| const client = useSupabaseClient() | ||
| const user = asyncComputed(async () => await client.auth.getUser().then(res => res.data.user)) |
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 is not a built-in helper, it's from VueUse, which doesn't seem to be a dependency of this demo - I do see it as one for the nuxt package though. Are we autoinstalling it for projects? Or is it just leaking into this project from the package?
I would consider dropping it for the demo to make the demo easier to adopt.
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, the module installs VueUse automatically as we are using it in some components. I wouldn't worry about including it since it's a very common 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.
This may be a slippery slope, generally we want our packages to be as slim as possible. If some of the other/bigger nuxt modules auto apply this dependency too maybe it's fine. But I wouldn't want us to be the only package that sneaks in a 1mb dependency that they didn't explicitly opt in for.
| "dependencies": { | ||
| "@nuxt/ui": "^4.0.0", | ||
| "@nuxtjs/supabase": "2.0.1", | ||
| "@powersync/nuxt": "workspace:*", |
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.
Our demos no longer depend on workspace dependencies, after this PR is approved we may need to split the work into two PRs to a release out that this demo can depend on.
This PR introduces a new
nuxtpackage to offer first-class support for Nuxt with PowerSync.The PR also introduces a new demo showcasing a reference implementation showing PowerSync + Nuxt 4 + Supabase