Conversation
… and initial setup
…Supabase Todo List demo
khawarizmus
requested review from
Chriztiaan,
LucDeCaf,
rkistner,
simolus3 and
stevensJourney
🦋 Changeset detectedLatest commit: 211b720 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Not sure what the chanset should include when releasing a new package |
stevensJourney
left a comment
stevensJourney
left a comment
There was a problem hiding this comment.
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.
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.
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.
Nit: We typically name these kinds of constants in upper case.
There was a problem hiding this comment.
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.
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.
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.
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.
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.
Just sharing this again. I'd really prefer we did not duplicate these implementations.
There was a problem hiding this comment.
Addressed above
packages/nuxt/src/runtime/composables/usePowerSyncInspectorDiagnostics.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
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.
…clarify diagnostics schema usage
…ate related constants
…spector-layout for improved loading experience
… usePowerSyncInspectorDiagnostics
…s in nuxt package
# Conflicts: # package.json # pnpm-lock.yaml
…-todolist configuration to use JWKS for authentication and enhance README with setup instructions
…ion and diagnostics support
… remove unused plugins
# Conflicts: # pnpm-lock.yaml
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| '@powersync/nuxt': minor | |||
There was a problem hiding this comment.
As we want this to be released under 0.0.1 for the first version, this will likely need to be a patch change instead.
|
|
||
| <NSectionBlock icon="carbon:data-share" text="Operations"> | ||
| <NTip v-if="!isDiagnosticSchemaSetup" n="red6 dark:red5" icon="carbon:warning-alt"> | ||
| Make sure to extend your schema with the diagnostics schema using the `diagnosticsSchema` from the |
There was a problem hiding this comment.
This is duplicated, weirdly. Is there room for reuse in this file?
4 participants
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