[Android] Make db size configurable

[alexkuttig]

Summary:

Since I do need a larger max db size, I added an optional size parameter to the package constructor. This makes the maximum db size configurable, while keeping the standard size of 6L * 1024L * 1024L when not giving any value. (like discussed in ##83)

new AsyncStoragePackage(100L * 1024L * 1024L)

Test Plan:

1.) create async storage in MainApplication.java of the test app with new AsyncStoragePackage()
2.) write > 6MB data to async storage
3.) change initialization of AsyncStorage in MainApplication.java to new AsyncStoragePackage(100L * 1024L * 1024L)
4.) write additional data

[krizzu]

Hey @alexkuttig, thanks for your input.

Like we pointed in discussion, Async Storage has been limited to 6MB by original authros, to avoid data loss (checkout this comment).

I believe this is a nice feature to have and use, if you know what potential consequences are.

We can go ahead and add it, but first, let's document it well. Can I ask you to do some more work here?

1. Go to docs/ in this project
2. Rename AdvancedUsage.md to BrownfieldIntegration.md
3. Create Advanced directory
4. Move BrownfieldIntegration.md to Advanced dir
5. Create new doc file (call it, for example, IncreaseDbSize.md or something similar)
6. Document your feature there - usage and potential consequences (you can quote original authors)

Let me know what you think.

thanks.

[alexkuttig]

Hi @krizzu,

thanks for your feedback. I added the documentation for this feature. Feedback or change requests welcome :-)

Best,
Alex

[krizzu]

Thanks a lot!

I've left my picky notes, please let me know what you think.

thanks.

[krizzu]

I'd put here sections for platforms, i.e use H2 for Android and below iOS (which is not supported for now).

[alexkuttig]

Thanks for your feedback! I added your request changes.

[krizzu]

@alexkuttig Did you see my comment here?

[tido64]

Is the link here still correct?

[tido64]

private?

[thymikee]

this is a bad practice to pass parameters to getPackages. It will also prevent autolinking available in RN 0.60 to work with this package. Please move the initialization logic to JS, something like:

// helpers/Storage.js
import {createAsyncStorage} from '@react-native-community/async-storage';
const AsyncStorage = createAsyncStorage(100 * 1024 * 1024);
export default AsyncStorage;

[krizzu]

Thanks @thymikee for pointing this out. I spoke with @Salakar and he explained me what's happening with auto-linking. Although, I'm not happy with configuring DB size from JS (especially that it's one platform related), but I've got an idea on how to work around this (@Salakar's suggestion™).

@alexkuttig I'll come back to you with necessary changes we have to do in order to get it working on new auto-linking feature (which will be backward compatible).

thanks

[krizzu]

@alexkuttig Auto-linking feature will not provide a way for devs to pass an argument to the Package constructor anymore (so we cannot pass new DB size like in your example). To work around this, let's use gradle to inject values in build time.

To get this working, we need to change where the default value comes from in ReactDatabaseSupplier.java, rest is handled in compile time.

I've implemented this in my fork.

So end user will just have to modify their gradle.properties to change size limit.

Let me know if you need any help.

thanks

[thymikee]

@krizzu @Salakar FYI I just realized (thanks to @grabbou finally providing documentation for the CLI config :P) that we have a dependency.platforms.android.packageInstance field (https://github.com/react-native-community/cli/blob/master/docs/dependencies.md#platformsandroidpackageinstance) for android that allows library maintainers to put custom variables to a constructor, that can be overriden in user's config dependencies['project-name'].platforms.android.packageInstance field (https://github.com/react-native-community/cli/blob/master/docs/projects.md#dependencies).

So technically this should be possible even though a bit cumbersome (and there are slight differences in configuration for pre RN 0.60). But still I wouldn't encourage such practices, it's more of an escape hatch for existing solutions. If the package can be zero or almost config, let's keep it that way.

[krizzu]

@thymikee Like you said, this would be more cumbersome than just adding a value in gradle.properties, so I stray off from that path. Thanks from bringing this up tho 👍

[alexkuttig]

Hey guys,

thanks for your feedback.
@krizzu that's a cool solution. How do we proceed? Do we just close this pull request and open yours, or should I change this one and add documentation for that feature?

[krizzu]

@alexkuttig I don't want your contribution to get lost, so please make changes in your current PR or close this one and open new one with docs and new implementation.

Thanks.

[alexkuttig]

Thats very kind of you. I made a new pull request (#99 )