feat: Added ExpoSQLiteAdapter and Code Sharing for common files

[chintannp]

Description of changes

This PR adds ExpoSQLiteAdapter inside datastore-storage-adapter package. Additionally, it includes updates to the SQLiteAdapter as few files have been moved under common directory for code sharing purpose.

Issue #, if available

Description of how you validated changes

Local testing by building a sample Todo App. More testing to be done.

Checklist

• PR description included
• yarn test passes
• Tests are changed or added
• Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[calebpollman]

Thanks for doing this @chintannp, definitely a good start. Left some feedback, feel free to reach out to me if you have any questions. Btw, while reviewing I noticed that we do not have @types/react-native-sqlite-storage which could help us improve some of the typing.

Edit: once we get further along it would be great to see unit tests added as well

[cshfang]

Overall actually lgtm! There are some questions and maybe gaps we need to address first but this PR is definitely coming along. I have a couple general thought about the Promise patterns being added. I see these pattern being used throughout:

public async foo(): Promise<any> {
  const resultSet = await new Promise((resolve, reject) => {
    resolve(res);
  })
  // build a result and then return it
  const result = resultSet.rows._array;
  return result;
}

public async bar(): Promise<any> {
  let result = [];
  await new Promise((resolve, reject) => {
    // push things into result then resolve the promise
    resolve();
  })
  return result;
}

But instead of wrapping a promise inside an async function just await it, we could/should refactor to just return these promises directly:

public async foo(): Promise<any> { // even the async keyword is technically optional, but it does still serve as a good hint that this function returns a promise to be run asynchronously
  return new Promise((resolve, reject) => {
    // build a result and then resolve it
    const result = res.rows._array;
    resolve(result);
  })
}

public async bar(): Promise<any> {
  let result = [];
  return new Promise((resolve, reject) => {
    // push things into result then resolve the promise
    resolve(result);
  })
}

Another thing that I think @calebpollman touched on is the error callbacks returning true. I think where there are not multiple statements to execute - we shouldn't need to add the return at all on the error callbacks. Where there is a desire to break out of a loop where multiple statements are being executed - I'm not sure true is the right thing to return either. Imo - just breaking with a return; should be sufficient.

[lgtm-com]

This pull request introduces 1 alert when merging 6a76400 into 9ddcc62 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 4 alerts when merging e478987 into 6117e71 - view on LGTM.com

new alerts:

• 4 for Unused variable, import, function or class

[cshfang]

I think this is looking much improved. Still have some feedback and questions but we're getting there!

[lgtm-com]

This pull request introduces 4 alerts when merging 9f3b666 into 1b18862 - view on LGTM.com

new alerts:

• 4 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 4 alerts when merging 07f16c0 into 1b18862 - view on LGTM.com

new alerts:

• 4 for Unused variable, import, function or class

[codecov-commenter]

Codecov Report

Merging #9581 (46cf8cd) into main (fe691fd) will decrease coverage by 0.01%.
The diff coverage is 67.53%.

@@            Coverage Diff             @@
##             main    #9581      +/-   ##
==========================================
- Coverage   80.44%   80.43%   -0.02%     
==========================================
  Files         255      255              
  Lines       18980    18974       -6     
  Branches     4111     4104       -7     
==========================================
- Hits        15269    15262       -7     
- Misses       3580     3581       +1     
  Partials      131      131              

Impacted Files	Coverage Δ
...-storage-adapter/src/common/CommonSQLiteAdapter.ts	64.90% <64.90%> (ø)
...atastore-storage-adapter/src/common/SQLiteUtils.ts	88.79% <73.91%> (ø)
...torage-adapter/src/SQLiteAdapter/SQLiteDatabase.ts	77.63% <75.75%> (-2.37%)	⬇️
...storage-adapter/src/SQLiteAdapter/SQLiteAdapter.ts	100.00% <100.00%> (+34.43%)	⬆️
.../datastore-storage-adapter/src/common/constants.ts	100.00% <100.00%> (ø)
packages/datastore-storage-adapter/src/index.ts

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[lgtm-com]

This pull request introduces 4 alerts when merging 26bed3c into 1b18862 - view on LGTM.com

new alerts:

• 4 for Unused variable, import, function or class

[calebpollman]

Good work so far, think this is really coming along. It would be great if we could find a way to reduce the amount of boilerplate code we are adding for the transaction handling (e.g. the result and error handling callbacks), maybe could look at adding a util function(s) that abstracts away the boilerplate?

[calebpollman]

Maybe I am being unclear. If this.db is falsy, then the block within this conditional will not execute. Do we need to inform the caller that nothing happened in this scenario?

[cshfang]

Lgtm

[calebpollman]

LGTM 🚢

[danrivett]

As an Expo user myself, I just wanted to say I've been waiting for this for quite a while, and so am very excited to see this has been implemented. Great work team, we're looking forward to trying it out once it's released. 🚀

[abdallahshaban557]

@danrivett - that is great to hear! We are excited to ship this out soon!

[Ashish-Nanda]

Thanks @danrivett. You can try it out right now on the @unstable tag and let us know if you have any feedback.

We'll be releasing it soon.

[chintannp]

@danrivett. Excited to hear feedback from you. Please use below steps to use the ExpoSQLiteAdapter.

1. Install the @aws-amplify/datastore-storage-adapter unstable version and additional dependencies using below command.

expo install @aws-amplify/datastore-storage-adapter@unstable expo-sqlite expo-file-system

2. Configure Datastore to use ExpoSQLiteAdapter as the default adapter for storage.

import { ExpoSQLiteAdapter } from '@aws-amplify/datastore-storage-adapter/ExpoSQLiteAdapter';

DataStore.configure({
  storageAdapter: ExpoSQLiteAdapter
});

[danrivett]

Thanks for the info @chintannp that's really helpful.

I'll pass that onto my developers to take a look. They are currently working on a separate AppSync/DataStore issue (see #9901), and then have an Expo 45 upgrade scheduled to complete, but testing the ExpoSQLiteAdapter has been scheduled after that as we only found out about it today.

So likely in a couple of weeks' time they'll start working on it, and I'll pass on any feedback I receive either here, or a separate issue if it seems to warrant it.

Thanks all.

[temaska]

Just wanted to try this adapter but import doesn't work at all. In a sample project:

datastore-storage-adapter version is 1.3.0

[abdallahshaban557]

@temaska - we are currently investigating this! Thank you for the feedback!

[chintannp]

@temaska ,
Thank you so much for reporting the issue! I have reported this as an issue here and will continue to actively work on this. We appreciate your feedback.

[chintannp]

@temaska ,
If you have typescript strict mode enabled. Can you try after disabling it ?

[temaska]

@chintannp ,
disabling strict mode removes this error but import warning still there.
The main issue is that even without typescript it imports 'undefined' instead of module. Just created a new sample project (without TS) from scratch and tried to add the module:

expo init AmplifyDataStoreExpo
cd AmplifyDataStoreExpo
npx amplify-app@latest
expo install aws-amplify @aws-amplify/datastore-storage-adapter expo-sqlite expo-file-system @react-native-community/netinfo @react-native-async-storage/async-storage

Then added the following code to App.js:

import { ExpoSQLiteAdapter } from '@aws-amplify/datastore-storage-adapter/ExpoSQLiteAdapter';

DataStore.configure({
  storageAdapter: ExpoSQLiteAdapter
});

export default function App() {
  console.log('Adapter: ', ExpoSQLiteAdapter)  
  return (
    <View style={styles.container}>
      <Text>Open up App.js to start working on your app!</Text>
      <StatusBar style="auto" />
    </View>
  );
}

No errors so far. But when executing this code (checked in Web and iOS) always receive 'Adapter: undefined' in console:

[chintannp]

@temaska, Yes, We need to make some improvements in regards to how the ExpoSQLiteAdapter is exported from the Submodule. We are working on to improve the typing, defining the datastore-storage-adapter/ExpoSQLiteAdapter as a sub-module. We will probably release a fix soon for this. In the meantime, you should be able to use the ExpoSQLiteAdapter by disabling the strict mode.

[chintannp]

@temaska Thank you for your feedback. We have fixed the problem and you can now use the ExpoSQLiteAdapter with strict mode enabled. You can install the latest version of @aws-amplify/datastore-storage-adapter and it should solve the error highlighting issue with the strict mode enabled.

Currently, the type for ExpoSQLiteAdapter will still be undefined. As there are no arguments that need to be passed to the ExpoSQLiteAdapter, it does not affect the functionality/usability of the adapter at all. We will continue to work to provide type support for ExpoSQLiteAdapter in the future.

[jpslav]

I was seeing the same undefined issue that @temaska was -- the problem, I believe, is that the Amplify docs do a named import when in fact the ExpoSQLiteAdapter is a default export. I made a PR to update the docs.