Skip to content
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

feat: remove CRSQLite and support MacOS in expo sqlite #32181

Merged

Conversation

coolsoftwaretyler
Copy link
Contributor

Why

I love using Expo wherever I can. I've recently been building with React Native for Windows and MacOS and wanted to use the Expo SQLite package.

I did some hacky patching a few weeks ago and wrote about it here. One of the big challenges I ran into was getting CRSQLite to play nice with everything once I updated the Podspec to work for MacOS.

With the upcoming CRSQLite deprecation and removal, I figured I'd bring it up to y'all. I put together a feature request with these details, and figured I'd at least put my patch findings into an actual PR that can be commented on, used, modified, or discarded if this isn't interesting.

How

This PR primarily does two things:

  1. Straightforward: lists macos in the platforms of the Podspec for Expo SQLite, which does just about everything a user would need to enable the library in a React Native MacOS project.
  2. Probably not mergeable: removes references and the XCFramework bundle for CRSQLite. This is certainly jumping the gun since it's just deprecated, not actually removed. But if SDK 53 is going to remove it, maybe this PR will line up with that.

Alternative to 2: I think to make it work without removing CRSQLite, we just need to add the Mac platform in the XCFramework bundle. I tried to build it from source and generate a new bundle but I couldn't get it to work, and with the upcoming deprecation/removal, I thought maybe a PR like this could just remove the library entirely, haha.

Test Plan

If you follow my blog post, you can step through the decisions I made for all these changes, along with a basic test that runs the basic CRUD operations code from a Mac app. I've seen this work myself, but if you folks want more thorough tests, I'd be happy to contribute with some guidance.

Checklist

Copy link
Contributor

Subscribed to pull request

File Patterns Mentions
docs/** @Simek, @amandeepmittal
packages/expo-sqlite/** @Kudo, @alanjhughes

Generated by CodeMention

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Oct 20, 2024
@tsapeta tsapeta requested review from Kudo and alanjhughes October 20, 2024 16:23
Copy link
Contributor

@Kudo Kudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for having the great pr!
i was hoping to keep cr-sqlite until sdk 53. to support macos, you could just specify the xcframework as ios only. then we don't have to remove cr-sqlite.

also be good to update the expo-module.config.js for ios -> apple
e.g.

{
  "name": "expo-sqlite",
  "platforms": ["apple", "android"],
  "apple": {
    "modules": ["SQLiteModule", "SQLiteModuleNext"]
  },
  "android": {
    "modules": ["expo.modules.sqlite.SQLiteModule", "expo.modules.sqlite.SQLiteModuleNext"]
  }
}

packages/expo-sqlite/ios/ExpoSQLite.podspec Outdated Show resolved Hide resolved
packages/expo-sqlite/ios/ExpoSQLite.podspec Show resolved Hide resolved
@coolsoftwaretyler
Copy link
Contributor Author

Thank you, @Kudo! I am at a work event this week but if I get some time to make these changes, I will. If not, I can get to it early next week.

Really appreciate everyone's time on this and I will keep an eye out for any additional change requests.

@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Oct 23, 2024
@coolsoftwaretyler
Copy link
Contributor Author

Thanks for the review, @Kudo! I think I have implemented the requested changes (including to bring back the crsqlite framework, and updating the changelog). Happy to make additional changes. Have a great day.

@coolsoftwaretyler
Copy link
Contributor Author

coolsoftwaretyler commented Oct 24, 2024

I think I forgot to restore one of the files. I'm able to reproduce the test failure locally for apps/bare-expo/scripts/start-ios-e2e-test.ts so I'll make sure to fix it before my next push.

Error is:

› Compiling BareExpo » SplashScreen.storyboard

❌  (ios/Pods/Target Support Files/Pods-BareExpoMain-BareExpo/ExpoModulesProvider.swift:215:7)

  213 |       SpeechModule.self,
  214 |       SQLiteModule.self,
> 215 |       SQLiteModuleNext.self,
      |       ^ cannot find 'SQLiteModuleNext' in scope
  216 |       StoreReviewModule.self,
  217 |       SymbolModule.self,
  218 |       ExpoSystemUIModule.self,

@coolsoftwaretyler
Copy link
Contributor Author

apps/bare-expo/scripts/start-ios-e2e-test.ts works locally if I revert the change to packages/expo-sqlite/expo-module.config.json, and the docs in docs/pages/modules/module-config.mdx don't seem to make mention of the apple keyword that was suggested. Is there another format or change I should make here?

Copy link
Contributor

@Kudo Kudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my fault to paste outdated expo-module.config.json. will update your pr and see if the ci getting passed

packages/expo-sqlite/expo-module.config.json Outdated Show resolved Hide resolved
packages/expo-sqlite/expo-module.config.json Outdated Show resolved Hide resolved
Copy link
Contributor

@Kudo Kudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine tune changelog

Copy link
Contributor

@Kudo Kudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here we go 🚀 thanks again for having the great contribution!

@Kudo Kudo requested a review from tsapeta October 25, 2024 08:59
@Kudo Kudo merged commit 66412ac into expo:main Oct 25, 2024
11 checks passed
Kudo added a commit that referenced this pull request Oct 25, 2024
Kudo added a commit that referenced this pull request Oct 25, 2024
# Why

fix outdated changes from #32181

# How

- move changelog to right place
- add macos to sdk 52 doc
@coolsoftwaretyler
Copy link
Contributor Author

Thank you for all the help! Y'all have a great contribution process here.

@brentvatne brentvatne added the published Changes from the PR have been published to npm label Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about published Changes from the PR have been published to npm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants