-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
[WIP] refactor: Persisted
state with storage adapter
#118
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: e1b2947 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 |
built with Refined Cloudflare Pages Action⚡ Cloudflare Pages Deployment
|
value: T; | ||
}; | ||
|
||
interface StorageAdapter<T> { |
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 think this type should be exported since it is part of the public options.
Also, what about making the contract asynchronous? This way one could write a storage adapter for IndexedDB.
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.
@webJose totally agree. This PR is a bit old as we were dealing with a bunch of rapid Svelte changes at the time. I'd like to be able to support generic storage adapters and asynchronous contracts to support IndexedDB or any other storage solution one could think of.
}); | ||
}); | ||
|
||
if (syncChanges) { |
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.
Should we make this a reactive option, so synchronization can be turned on/off on the fly? Is there any value in having this capability?
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 haven't stumbled upon such a scenario in a real application. However, sounds interesting. Maybe is good for those creative thinkers out there that may use local storage to sync multiple windows. This could be used from debugging to fancier connect/disconnect windows.
`Error when writing value from persisted store "${key}" to ${storage.constructor.name}`, | ||
e | ||
); | ||
} |
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 think we only want to log here during dev.
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.
In svelte-persisted-store
there is an onerror
option (or similar) that accepts a function. This is nice for people submitting logs to structured log destinations, like Seq. The other options can be to "warn" or to "ignore"? Some of us hate packages soiling the console output.
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.
In svelte-persisted-store
there is an onerror
option (or similar) that accepts a function. This is nice for people submitting logs to structured log destinations, like Seq. The other options can be to "warn" or to "ignore"? Some of us hate packages soiling the console output.
value: T; | ||
}; | ||
|
||
interface StorageAdapter<T> { |
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.
@webJose totally agree. This PR is a bit old as we were dealing with a bunch of rapid Svelte changes at the time. I'd like to be able to support generic storage adapters and asynchronous contracts to support IndexedDB or any other storage solution one could think of.
Also there's now the |
@huntabyte Just a heads up I don't plan on doing anything with this PR. Please feel free to take over/branch/close as you see fit :) |
No description provided.