-
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.
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 have added all necessary files and instructions for this
| @@ -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.
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 exported the names of the tables as a cost.
| /** | ||
| * 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
| // 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.
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 have used the SharedWebStreamingSyncImplementation, which made the inspector work across tabs now. but because it is the only way to have it work with the official Nuxt devtools, I opted into overriding the settings to force multitab.
Another way would be to display an error and say that they have to use multitab but can still access the inspector via direct URL. This would require detecting that we are inside the devtool (which I think is possible)
| 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
scripts/reset-node-modules.ts
Outdated
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 ?
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 have renamed the script to reset-repo and added it to the packagejson` file.
kept the logic and made it delete pnpm-lock files. I was thinking of making it also delete dist and lib folders as well as tsconfig.tsbuildinfo files. This would make it easier to reset -> install -> build
Let me know what you guys think
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 like that idea. One thing to be wary of if we delete lib files is that some projects use/used lib as a source folder instead of a build folder. (IIRC I replaced one of those with a library folder, not sure if there's still more but worth considering.)
It might also be worth letting users choose which repos are cleaned. There's some similar looking logic in the demo management scripts if you feel like implementing this.
…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.
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 have made it so that the user has to explicitly opt in to use Kysely, and the composable won't be imported unless they do. The dependency is also optional, and if they opt in and don't have it installed, the module would error.
| @@ -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.
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.
Good point. I have made things more consistent now.
| 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
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 addressed
| 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.
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.
Can you confirm which version is applied if you install a differing VueUse version in the demo too? Conflicts, or is one prioritised over the other?
| "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.
…clarify diagnostics schema usage
…ate related constants
…s, and remove obsolete reset-node-modules script
# Conflicts: # packages/web/src/index.ts # pnpm-lock.yaml
…, improving code readability and structure
…spector-layout for improved loading experience
… usePowerSyncInspectorDiagnostics
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