-
Notifications
You must be signed in to change notification settings - Fork 61
Isolate demos folder
#784
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?
Isolate demos folder
#784
Conversation
|
|
Can we add demo lockfiles to |
05f2c98 to
e088e7d
Compare
fe023f5 to
80c3199
Compare
Chriztiaan
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.
Last bit of reverting version drops, then it can go in.
LucDeCaf
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.
Yep, fixed the misversioned packages
|
Happy with the changes. |
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.
I can see a lot of hard work has gone into this. This is already a large improvement from the previous setup.
I added some comments for some potential improvements to make it potentially even better.
| "@angular/service-worker": "^19.2.4", | ||
| "@journeyapps/wa-sqlite": "^1.4.0", | ||
| "@powersync/web": "workspace:*", | ||
| "@powersync/web": "^1.29.1", |
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.
There appears to be some inconsistency between demos. E.g. here the version is specified as ^1.29.1, while the example-capacitor demo has ^1.30.0. Should the script be executed again here?
| "@powersync/common": "workspace:*", | ||
| "@powersync/react": "workspace:*", | ||
| "@powersync/react-native": "workspace:*", | ||
| "@powersync/common": "^1.43.1", |
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 see it was here before, but, do we need the @powersync/common dependency here? I'd hope it would be resolved by @powersync/react-native
| @@ -0,0 +1 @@ | |||
| declare module 'better-sqlite3'; | |||
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.
Could we use @types/better-sqlite3 for this?
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.
| "@powersync/op-sqlite": "^0.7.16", | ||
| "@powersync/react": "^1.8.2", | ||
| "@powersync/react-native": "^1.27.1", | ||
| "@powersync/common": "^1.43.1", |
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 suppose we should remove this as a dependency from all demos that might not require it directly.
| () => { | ||
| if (!session) getSession(); | ||
| }, | ||
| // [session, 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.
For my context: Why is the user dependency commented out 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 simply forgot to delete it after testing if it worked.
| const workspacePackages = await findWorkspacePackages(path.resolve('.')); | ||
|
|
||
| // Function to split user-provided demos into found and not found demos | ||
| const filterDemos = (allDemos: string[], providedDemos: string[]): [string[], 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.
A bit of a nit: The tuple result here, could be in my opinion, a bit neater if we returned a keyed object e.g. {found: string[], notFound: string[]}
| }; | ||
|
|
||
| for (const arg of process.argv.slice(2)) { | ||
| if (arg === '--no-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.
Just mentioning again, something like Commander would provide details and documentation for the supported arguments if a user executed this script with --help or provided an invalid value.
| const allDemos = fs.readdirSync(demosDir); | ||
| let demoNames: string[]; | ||
|
|
||
| if (args.length > 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.
Nit: One could perhaps use something like https://www.npmjs.com/package/inquirer for an optional interactive selection (for those of us who are too lazy to type the full demo name).
| process.exit(0); | ||
| } | ||
|
|
||
| console.log('\nInstalling packages...'); |
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 got this error when running the script locally
Updating packages...
- @powersync/react: '^1.8.2' => 'workspace:*'
- @powersync/web: '^1.29.1' => 'workspace:*'
Linking tsconfig.demo.json
- Added '{"path":"../../tsconfig.demo.json"}'
Done.
Installing packages...
Scope: all 18 workspace projects
Lockfile is up to date, resolution step is skipped
Packages: -374
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Progress: resolved 0, reused 201, downloaded 0, added 0, done
╭ Warning ────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ │
│ Ignored build scripts: @journeyapps/wa-sqlite, @swc/core, core-js, core-js-pure, detox, esbuild, swiftlint. │
│ Run "pnpm approve-builds" to pick which dependencies should be allowed to run scripts. │
│ │
╰─────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
Done in 2.5s using pnpm v10.8.0
Done.
undefined:1
/* To learn more about this file see: https://angular.io/config/tsconfig. */
^
SyntaxError: Unexpected token '/', "/* To lear"... is not valid JSON
at JSON.parse (<anonymous>)
at linkDemo (file:///Users/stevenontong/Documents/platform_code/powersync/powersync-react-native-sdk/scripts/dist/demos-use-workspace.js:69:31)
at main (file:///Users/stevenontong/Documents/platform_code/powersync/powersync-react-native-sdk/scripts/dist/demos-use-workspace.js:127:9)
at file:///Users/stevenontong/Documents/platform_code/powersync/powersync-react-native-sdk/scripts/dist/demos-use-workspace.js:143:1
Node.js v20.9.0
It might be related to this JSON in a demo's tsconfig.json

Could you also please give some context for how this script works.
I'm a bit confused by the current structure. Where:
- The demos have their own
pnpm-workspace.yamlfile which defines the demo folder as its own monorepo (in order to avoid issues from nested workspaces I presume) - The monorepo root now does not include the
demosfolder as part of its workspace (and we don't change that with this script) - This script updates the demo's package.json dependencies to use
workspace:*for packages defined in the root workspace
If I run the script for a single demo like the react-supabase-todolist demo. The @powersync/web package is indeed located in the demos/react-supabase-todolist/node_modules folder - I'm not sure why this happens, since we execute a pnpm install command in the repo root which does not include the demos folder in the workspace. However, if I clear my node modules for the entire repo with rm -rf **/node_modules and do pnpm install in the root, then the web sdk is not present in the demo's node_modules folder. If I try to run pnpm install in the demo, I get the following (expected) error
❯ pnpm install
ERR_PNPM_WORKSPACE_PKG_NOT_FOUND In : "@powersync/react@workspace:*" is in the dependencies but no package named "@powersync/react" is present in the workspace
This could be problematic, if for example a dev wants to even install or update a different dependency in a demo while developing.
Furthermore. This method of updating the Git tracked package.json files with workspace:* dependencies would result in changes which are hard to avoid committing during development. For this reason alone, I can say I would not use this script in my personal development.
I would rather use the npm link command inside the demo, e.g.
npm link # in packages/web
npm link @powersync/web # in demo
Note the above uses NPM (not PNPM) since PNPM has historically been tricky with linking. PNPM links will also modify tracked Git files. There might be some issues for using NPM with the local PNPM workspace (haven't thoroughly tested this myself)
That being said, if one wants to use PNPM and have linking without modifying tracked source files, it is also possible by using dynamic overrides in a pnpmfile
Update: After typing this comment, I noticed there are instructions for adding the demos folder to the root workspace and using git restore to clear changes to tracked files. This is probably personal preference, but I still would not want to run git restore each time before committing during development.
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.
To clarify on .pnpmfile.cjs - is the idea to have a development script that generates a .pnpmfile.cjs in each demo which intercepts the installation of '@powersync/*' packages by linking to the local versions instead of the published versions?
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.
On a high-level, that is what would be done. There are a few options to actually implement this.
The .pnpmfile.cjs could contain the relevant dependency overrides in the file, or it could read the overrides from another file - e.g. a json file which contains a Map of package names and paths to each package to be overriden.
Generally, I think having a standard pnpmfile.cjs implementation which reads the overrides dynamically from a JSON file is cleaner - that way one does not need to edit JavaScript files to configure which packages are to be linked.
The linking script could create the pnpmfile in the demo folder, or the file could always be present. I don't really have a strong opinion on this.
| return result; | ||
| }; | ||
|
|
||
| const main = () => { |
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.
We should add this script to the changeset:version script. This would ensure that the demos are updated as part of the release process.
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.
For added peace-of-mind, once could also do a changeset version in the Test Isolated Demos action, before running the Verdaccio publishing. This could help catch versioning and package issues sooner.

Currently, the demos in the
demosfolder use pnpm's workspace feature to link to workspace packages. This is convenient for testing changes to workspace packages, as demos will automatically use the local version - however, it causes issues for other users of the demos, as they are required to install all demos and all packages' dependencies in order to run even a single demo. Also, copying demos out of the monorepo requires one to figure out the correct package versions manually.This PR replaces the
@powersync/*package versions fromworkspace:*to the latest published releases and adds apnpm-workspace.yamlfile to each demo. This isolates the demos from the rest of the workspace and ensures that each demo can run independently from thepowersync-jsrepo's context. For convenience, this PR also adds a set of scripts for re-linking the@powersync/*packages to the workspace (for ease of development and testing) and for upgrading@powersync/*package versions across demos.Importantly, this PR also includes a partial rewrite of the
react-native-supabase-group-chatdemo, which was using old packages and failing to build on my system. The PR which made these changes was merged into this branch, since it also used published PowerSync package versions instead of workspace versions.