-
Notifications
You must be signed in to change notification settings - Fork 26
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
[Fix] Common SDK fixes for Web and React Native #24
Conversation
packages/powersync-sdk-common/src/client/AbstractPowerSyncDatabase.ts
Outdated
Show resolved
Hide resolved
* This provides a top-level read only execute method which is executed inside a read-lock. | ||
* This is necessary since the high level `execute` method uses a write-lock under | ||
* the hood. Helper methods such as `get`, `getAll` and `getOptional` are read only, | ||
* and should use this method. |
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.
What is a top-level
read-only execute method?
According to the comment, get
, getAll
and getOptional
should use this, but it does not seem to be the 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.
The DB adapter and the powersync client expose SQL execution methods such as execute
, getAll
, get
and getOptional
on the class (top-level) e.g. db.execute()
, db.get()
and they also expose these methods in lock and transaction contexts e.g. tx.get()
.
The top-level get
functions were previously using the top-level execute
function which would actually schedule a writeLock and then use the writeLock's context execute
method. These methods should actually be using a readLock
instead since those can execute concurrently with writeLocks. This method provides a read only execute
method on the DB adapter class as shorthand for starting a readLock and using it's execute
method.
This method is used to create the top-level get
helpers here https://github.com/journeyapps/powersync-react-native-sdk/blob/fc2eb5d9bf74e71ee3cd7fe896ecba5ca4f27081/packages/powersync-sdk-react-native/src/db/adapters/react-native-quick-sqlite/RNQSDBAdapter.ts#L30
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.
100%
This PR contains some fixes identified when implementing the Web SDK:
get
,getAll
andgetOptional
helper methods should execute inside a readLock for better concurrencyuser-id
header from backend connector and remote headersThe user-id field is not used and is not accepted for CORS
waitForReady
method on PowerSyncDatabaseany
.@journeyapps/react-native-quick-sqlite
which has bug fixes from [Fix] Missing dependencies react-native-quick-sqlite#9TODO: