-
Notifications
You must be signed in to change notification settings - Fork 61
OPFS Multiple Tab Trigger Invocation #804
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?
Changes from all commits
9637aaf
f66ce86
22877b6
fcfcc32
24ac097
dcba009
f437400
797bde5
d88e630
fa6f129
7150f96
db14b39
5ea40a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| '@powersync/common': minor | ||
| '@powersync/web': minor | ||
| --- | ||
|
|
||
| Add support for storage-backed (non-TEMP) SQLite triggers and tables for managed triggers. These resources persist on disk while in use and are automatically cleaned up when no longer claimed or needed. They should not be considered permanent triggers; PowerSync manages their lifecycle. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@powersync/web': minor | ||
| --- | ||
|
|
||
| Managed triggers now use storage-backed (non-TEMP) SQLite triggers and tables when OPFS is the VFS. Resources persist across tabs and connection cycles to detect cross‑tab changes, and are automatically cleaned up when no longer in use. These should not be treated as permanent triggers; their lifecycle is managed by PowerSync. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import { TriggerClaimManager } from './TriggerManager.js'; | ||
|
|
||
| /** | ||
| * @internal | ||
| * @experimental | ||
| */ | ||
| export class MemoryTriggerClaimManager implements TriggerClaimManager { | ||
| // Uses a global store to share the state between potentially multiple instances | ||
| private static CLAIM_STORE = new Map<string, () => Promise<void>>(); | ||
|
|
||
| async obtainClaim(identifier: string): Promise<() => Promise<void>> { | ||
| if (MemoryTriggerClaimManager.CLAIM_STORE.has(identifier)) { | ||
| throw new Error(`A claim is already present for ${identifier}`); | ||
| } | ||
| const release = async () => { | ||
| MemoryTriggerClaimManager.CLAIM_STORE.delete(identifier); | ||
| }; | ||
| MemoryTriggerClaimManager.CLAIM_STORE.set(identifier, release); | ||
|
|
||
| return release; | ||
| } | ||
|
|
||
| async checkClaim(identifier: string): Promise<boolean> { | ||
| return MemoryTriggerClaimManager.CLAIM_STORE.has(identifier); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,8 +47,9 @@ export interface BaseTriggerDiffRecord<TOperationId extends string | number = nu | |
| * This record contains the new value and optionally the previous value. | ||
| * Values are stored as JSON strings. | ||
| */ | ||
| export interface TriggerDiffUpdateRecord<TOperationId extends string | number = number> | ||
| extends BaseTriggerDiffRecord<TOperationId> { | ||
| export interface TriggerDiffUpdateRecord< | ||
| TOperationId extends string | number = number | ||
| > extends BaseTriggerDiffRecord<TOperationId> { | ||
| operation: DiffTriggerOperation.UPDATE; | ||
| /** | ||
| * The updated state of the row in JSON string format. | ||
|
|
@@ -65,8 +66,9 @@ export interface TriggerDiffUpdateRecord<TOperationId extends string | number = | |
| * Represents a diff record for a SQLite INSERT operation. | ||
| * This record contains the new value represented as a JSON string. | ||
| */ | ||
| export interface TriggerDiffInsertRecord<TOperationId extends string | number = number> | ||
| extends BaseTriggerDiffRecord<TOperationId> { | ||
| export interface TriggerDiffInsertRecord< | ||
| TOperationId extends string | number = number | ||
| > extends BaseTriggerDiffRecord<TOperationId> { | ||
| operation: DiffTriggerOperation.INSERT; | ||
| /** | ||
| * The value of the row, at the time of INSERT, in JSON string format. | ||
|
|
@@ -79,8 +81,9 @@ export interface TriggerDiffInsertRecord<TOperationId extends string | number = | |
| * Represents a diff record for a SQLite DELETE operation. | ||
| * This record contains the new value represented as a JSON string. | ||
| */ | ||
| export interface TriggerDiffDeleteRecord<TOperationId extends string | number = number> | ||
| extends BaseTriggerDiffRecord<TOperationId> { | ||
| export interface TriggerDiffDeleteRecord< | ||
| TOperationId extends string | number = number | ||
| > extends BaseTriggerDiffRecord<TOperationId> { | ||
| operation: DiffTriggerOperation.DELETE; | ||
| /** | ||
| * The value of the row, before the DELETE operation, in JSON string format. | ||
|
|
@@ -201,6 +204,12 @@ interface BaseCreateDiffTriggerOptions { | |
| * Hooks which allow execution during the trigger creation process. | ||
| */ | ||
| hooks?: TriggerCreationHooks; | ||
|
|
||
| /** | ||
| * Use storage-backed (non-TEMP) tables and triggers that persist across sessions. | ||
| * These resources are still automatically disposed when no longer claimed. | ||
| */ | ||
| useStorage?: boolean; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that we have mechanisms to clean up those triggers reliably (albeit after a small delay), I wonder if it could make sense to unconditionally use stored triggers and to drop support for in-memory triggers? The only benefit of in-memory triggers is potentially better performance. But not having this as an option would allow us to remove additional complexity in opening logic added to the web database. So if the performance benefit is not that significant, reducing implementation complexity might be a good tradeoff.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a fair point to mention this. There probably are a few scenarios where we want to avoid increasing the storage size. Two which come to mind are:
We could potentially perform the inverse, where storage is used by default and disabled for those cases - but then we still need to be able to configure the defaults and lose some of those potential performance benefits. |
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -449,3 +458,30 @@ export interface TriggerManager { | |
| */ | ||
| trackTableDiff(options: TrackDiffOptions): Promise<TriggerRemoveCallback>; | ||
| } | ||
|
|
||
| /** | ||
| * @experimental | ||
| * @internal | ||
| * An interface which exposes which persisted managed SQLite triggers and destination SQLite tables | ||
| * are actively in use. Resource which are not reported as claimed by this interface will be disposed. | ||
| */ | ||
|
Comment on lines
+462
to
+467
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The PR description gives a very nice overview of what this interface is supposed to accomplish, I think copying parts of the "The Claim Mechanism" section into this documentation comment might make the purpose of this easier to understand for someone stumbling upon this later. Or at least for me this comment is a bit abstract and doesn't make it obvious why this is necessary. |
||
|
|
||
| export interface TriggerClaimManager { | ||
| /** | ||
| * Obtains or marks a claim on a certain identifier. | ||
| * @returns a callback to release the claim. | ||
| */ | ||
| obtainClaim: (identifier: string) => Promise<() => Promise<void>>; | ||
| /** | ||
| * Checks if a claim is present for an identifier. | ||
| */ | ||
| checkClaim: (identifier: string) => Promise<boolean>; | ||
| } | ||
|
|
||
| /** | ||
| * @experimental | ||
| * @internal | ||
| */ | ||
| export interface TriggerManagerConfig { | ||
| claimManager: TriggerClaimManager; | ||
| } | ||
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: Having a class whose only internal state is a static field feels a bit odd to me, perhaps a global constant implementing
TriggerClaimManageras a JS object would make that clearer.