Removing Expo support #26
Conversation
# Conflicts: # RNBatchPush.podspec # plugin/src/withReactNativeBatch.ts
# Conflicts: # plugin/src/__tests__/withReactNativeBatchMainActivity.test.ts # plugin/src/android/withReactNativeBatchMainActivity.ts # plugin/src/fixtures/mainActivity.ts # plugin/src/withReactNativeBatch.ts
| **Core** | ||
| - Batch no longer requires a custom React Native CLI configuration. If `react-native.config.js` only exists for Batch, delete it or remove the `@batch.com/react-native-plugin` entry. | ||
|
|
||
| **Android** |
There was a problem hiding this comment.
Pas de changement pour iOS ?
There was a problem hiding this comment.
nan on a pas de breaking change coté ios, l'init du plugin était déjà manuelle et l'api à pas changée.
|
@codex review |
ThomasMengelatte
left a comment
ThomasMengelatte
left a comment
There was a problem hiding this comment.
Quel bonheur de voir les creepy tests sur l'appdelegate deleted <3 de voir
| String batchAPIKey = resources.getString(resources.getIdentifier("BATCH_API_KEY", "string", packageName)); | ||
| Batch.start(batchAPIKey); | ||
| Batch.EventDispatcher.addDispatcher(eventDispatcher); | ||
| try { |
There was a problem hiding this comment.
Un interet particulier de le déplacer dans setDefaultConfig ? ou c'est pour la clareté du code ?
There was a problem hiding this comment.
ouai parce que la valeur est lue via le manifest maintenant et plus par les string resources (sinon ca faisait utiliser un dangerous mod coté expo), donc du tout j'uniformise toute les valeurs lue via le manifest au même endroit (d'ou le rename de la méthode).
| try { | ||
| Bundle metaData = context.getPackageManager() | ||
| .getApplicationInfo(packageName, PackageManager.GET_META_DATA) | ||
| .metaData; | ||
| if (metaData != null) { | ||
| // DnD Initial State | ||
| boolean doNotDisturbEnabled = metaData.getBoolean("batch_do_not_disturb_initial_state", false); |
There was a problem hiding this comment.
Plus besoin du try ? pour le set a false ?
There was a problem hiding this comment.
nan la méthode à un fallback
There was a problem hiding this comment.
ah oui j'avais pas vu 👍
| if (app != null) { | ||
| initialize(app); | ||
| } else { | ||
| Log.e(LOGGER_TAG, "Application context is null, cannot initialize Batch module"); |
There was a problem hiding this comment.
Utilse de continuer si le context est null ?
There was a problem hiding this comment.
normalement on passe jamais là, c'est juste dans le "doute"
ios/RNBatch.mm
Outdated
| #ifdef RCT_NEW_ARCH_ENABLED | ||
| #import <RNBatchSpec/RNBatchSpec.h> | ||
| @interface RNBatchModule: RCTEventEmitter <NativeRNBatchModuleSpec> | ||
| #else |
There was a problem hiding this comment.
On veut pas drop l'anciene ?
There was a problem hiding this comment.
c'est dans une autre PR
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This PR remove the Expo support from this repository since it's being migrate to a new dedicated expo plugin (using Expo Modules APIs).
It also rework the android setup, since we dont want to use a custom React Native CLI configuration anymore.
Drop of the support of the old arch is coming in another PR.