-
Notifications
You must be signed in to change notification settings - Fork 60
feat: add modular resource fetcher adapters for Expo and bare React Native #759
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?
feat: add modular resource fetcher adapters for Expo and bare React Native #759
Conversation
Add modular resource fetcher adapters to support both Expo and bare React Native environments. ## New Packages ### @rn-executorch/expo-adapter - Expo-based resource fetcher using expo-file-system - Supports asset bundles, local files, and remote downloads - Download management with pause/resume/cancel capabilities ### @rn-executorch/bare-adapter - Bare React Native resource fetcher using RNFS and background downloader - Supports all platform-specific file operations - Background download support with proper lifecycle management ## Core Changes - Refactor ResourceFetcher to use adapter pattern - Add initExecutorch() and cleanupExecutorch() for adapter management - Export adapter interfaces and utilities - Update LLM controller to support new resource fetching ## App Updates - Update computer-vision, llm, speech-to-text, text-embeddings apps - Add adapter initialization to each app - Update dependencies to use workspace packages
Add a complete bare React Native example app demonstrating LLM integration with react-native-executorch. ## App: llm_bare ### Features - Simple chat UI for LLM interactions - Model loading with progress indicator - Real-time streaming responses - Send/stop generation controls - Auto-scrolling message history ### Stack - **Framework**: React Native 0.81.5 (bare/CLI) - **LLM**: Uses LLAMA3_2_1B_SPINQUANT model - **Adapter**: @rn-executorch/bare-adapter - **Dependencies**: Minimal deps, only essential packages ### Platform Configuration #### iOS - Bridging header for RNBackgroundDownloader - Background URL session handling in AppDelegate - Background modes (fetch, processing) - Xcode project configuration #### Android - Required permissions for background downloads - Foreground service configuration - Network state access - Proper manifest configuration ### Infrastructure - Babel configuration for export namespace transform This serves as a reference implementation for using react-native-executorch in bare React Native environments (non-Expo).
|
Lint CI fails (you don't need to worry about the second failing CI ;)). Please could you fix the errors from this CI? |
|
@msluszniak I’m not able to reproduce the lint CI failure locally — everything passes on my side. I’ll take a closer look and investigate further to see what might be causing the discrepancy (environment, cache, or config differences). I’ll follow up with a fix or more details as soon as I find the root cause.
|
|
@rizalibnu Sure thing, maybe the configuration of the CI itself does not work with as it should. We'll also look at this, don't worry :) |
|
@msluszniak Found the issue 👍 Fixed by adding a build step before adapter type checks and bumped Node to 22 in |
…package.json for bare and expo adapters
…r for better error handling
72914c0 to
359427b
Compare
package.json
Outdated
| "apps/computer-vision", | ||
| "apps/llm", | ||
| "apps/speech", | ||
| "apps/text-embeddings" |
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.
Why we need to expanded apps/*
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.
@msluszniak We need to expand apps/* because apps/bare_rn in PR #763 must be excluded from the Yarn workspace. An alternative is explicitly excluding it in package.json as shown below.
"workspaces": [
"apps/*",
"!apps/bare_rn"
]Which approach do you prefer?
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.
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 agree with the alternative 👍
Using apps/* with an explicit exclusion is more future-proof.
Should we apply the same glob + exclusion pattern to packages/ as well for consistency?
"workspaces": {
"packages": [
"packages/*",
"apps/*",
"!apps/bare_rn"
]
}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 I'm on the alternative side as we likely won't add much bare RN example apps in the future.
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’ve applied the glob pattern to apps/ and packages/. Please re-review. @msluszniak @chmjkb
| @@ -0,0 +1,34 @@ | |||
| { | |||
| "name": "@rn-executorch/expo-adapter", | |||
| "version": "0.1.0", | |||
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.
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 that's fine. We just need to keep in mind to add a compatibility table, if some version is not compatible at some point. We can also make sure that RNET is a peer dependency of the adapters so we can narrow down on the versions there.
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 just remember from one project, that we wanted at some point unify the versions of packages in monorepo, but it was to late since we started with the approach as here. Same thing as Apple did with OS versions on multiple devices ;p
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.
That’s a fair concern.
While independent adapter releases are useful early on, IMO I think a good compromise might be to align major/minor versions between adapters and RNET, while allowing independent patch releases. This keeps compatibility clear for users without blocking adapter-only fixes.
Happy to follow whatever versioning direction the maintainers prefer.
apps/llm/tsconfig.json
Outdated
| "paths": { | ||
| "react-native-executorch": ["../../packages/react-native-executorch/src"] | ||
| "react-native-executorch": ["../../packages/react-native-executorch/src"], | ||
| "@rn-executorch/expo-adapter": ["../../packages/expo-adapter/src"] |
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: we're likely going to call the package react-native-executorch/expo-resource-fetcher or react-native-executorch/bare-resource-fetcher, so let's stick to that naming 👍🏻
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.
Updated. Please re-review. @chmjkb
package.json
Outdated
| "apps/computer-vision", | ||
| "apps/llm", | ||
| "apps/speech", | ||
| "apps/text-embeddings" |
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 I'm on the alternative side as we likely won't add much bare RN example apps in the future.
| } | ||
|
|
||
| export function cleanupExecutorch() { | ||
| ResourceFetcher.setAdapter(null as unknown as ResourceFetcherAdapter); |
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.
Maybe it would be a tiny bit cleaner if we had a resetAdapter or something like this exposed from ResourceFetcher
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’ve added resetAdapter. Please re-review. @chmjkb
packages/bare-adapter/README.md
Outdated
| - Projects that need true background downloads | ||
| - Projects requiring direct native filesystem access | ||
|
|
||
| This adapter uses `@dr.pogodin/react-native-fs` and `@kesha-antonov/react-native-background-downloader` for enhanced file operations and background download support. |
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.
Maybe it would be cool to link here to their installation steps
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.
Updated. Please re-review. @chmjkb
|
|
||
| export async function createDirectoryIfNoExists() { | ||
| if (!(await checkFileExists(RNEDirectory))) { | ||
| await RNFS.mkdir(RNEDirectory); |
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.
Can we try catch here and re-throw rnexecutorch erorr?
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.
Updated. Please re-review. @chmjkb
| extendedInfo: ResourceSourceExtended; | ||
| } | ||
|
|
||
| export namespace ResourceFetcherUtils { |
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.
Is there a reason for exporting this as a namespace?
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.
@chmjkb It follows the original implementation in the existing codebase. I kept the same pattern to stay consistent and avoid unnecessary refactors.
https://github.com/software-mansion/react-native-executorch/blob/main/packages/react-native-executorch/src/utils/ResourceFetcherUtils.ts#L50
| pauseFetching(...sources: ResourceSource[]): Promise<void>; | ||
| resumeFetching(...sources: ResourceSource[]): Promise<void>; | ||
| cancelFetching(...sources: ResourceSource[]): Promise<void>; | ||
| listDownloadedFiles(): Promise<string[]>; | ||
| listDownloadedModels(): Promise<string[]>; | ||
| deleteResources(...sources: ResourceSource[]): Promise<void>; | ||
| getFilesTotalSize(...sources: ResourceSource[]): Promise<number>; | ||
| readAsString(path: string): Promise<string>; |
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'm wondering if these should be enforced, in the library internals, we only use fetch(), meaning that in case where the user wants to implement his own resource fetcher, he only has to implement fetch() for the library internals to work. The other methods are just for convenience. We should either add a JSDoc here saying that or we should remove them from this interface completely. cc @mkopcins, wdyt?
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.
yes, we should require the bare minimum for the fetcher to work and mark other method as optional to make implementing custom fetcher as simple as possible
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.
Add explicit resetAdapter() method to ResourceFetcher class for cleaner API. - Add resetAdapter() static method that sets adapter to null - Update cleanupExecutorch() to use resetAdapter() instead of type assertion hack - Update error message to reference new package names (@react-native-executorch/*) This provides a cleaner, type-safe way to reset the adapter without requiring "null as unknown as ResourceFetcherAdapter" type assertion.
…ecking and cleaning

Description
This PR introduces modular resource fetcher adapters to support both Expo and bare React Native environments, replacing the previous monolithic approach with a flexible, platform-specific architecture.
Key Changes
New Adapter Packages:
Initialization Changes:
Documentation Updates:
Introduces a breaking change?
Migration Required:
Users must now explicitly initialize the library with a resource fetcher adapter:
Type of change
Tested on
Testing instructions
For Expo projects:
yarn add @rn-executorch/expo-adapter expo-file-system expo-assetinitExecutorch({ resourceFetcher: ExpoResourceFetcher })For bare React Native projects:
yarn add @rn-executorch/bare-adapter @dr.pogodin/react-native-fs @kesha-antonov/react-native-background-downloaderinitExecutorch({ resourceFetcher: BareResourceFetcher })Screenshots
Related issues
Closes #549
Checklist
Additional notes
Why This Change:
Split Into Multiple PRs:
To make review easier, this work has been split:
BREAKING CHANGE:
initExecutorch()with explicit adapter selection is now required before using any react-native-executorch hooks. Users must install and configure either@rn-executorch/expo-adapteror@rn-executorch/bare-adapterdepending on their project type.