-
Notifications
You must be signed in to change notification settings - Fork 35
[MOB-11175] make-rn-sdk-compatible-with-expo #629
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
[MOB-11175] make-rn-sdk-compatible-with-expo #629
Conversation
… multiple files to use NativeModules directly, as expo does breaks when importing this from another file
…, as the empty class was breaking expo
…rwise there appears to be an error in expo
…bleInAppManager and IterableInboxDataModel, otherwise expo gets upset
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.
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
❌ 3 blocking issues (4 total)
@qltysh one-click actions:
This is from Qlty Cloud, the successor to Code Climate Quality. Learn more. |
import { NativeModules } from 'react-native'; | ||
|
||
import { Iterable } from '../../core'; | ||
import type { IterableInAppDeleteSource, IterableInAppLocation } from '../enums'; |
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.
unused expression, expected an assignment or function call
import { NativeModules } from 'react-native'; | ||
|
||
import { Iterable } from '../../core'; | ||
import type { IterableInAppDeleteSource, IterableInAppLocation } from '../enums'; |
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.
Do not use comma operator here because it can be easily misunderstood or lead to unintended bugs.
IterableInboxImpressionRowInfo, | ||
IterableInboxRowViewModel, | ||
} from '../types'; | ||
import type { IterableInboxImpressionRowInfo, IterableInboxRowViewModel } from '../types'; |
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.
unused expression, expected an assignment or function call
IterableInboxImpressionRowInfo, | ||
IterableInboxRowViewModel, | ||
} from '../types'; | ||
import type { IterableInboxImpressionRowInfo, IterableInboxRowViewModel } from '../types'; |
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.
Do not use comma operator here because it can be easily misunderstood or lead to unintended bugs.
IterableInAppManager, | ||
IterableInAppMessage, | ||
} from '../../inApp'; | ||
// TODO: Organize these so that there are no circular dependencies |
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.
TODO found
// expo throws an error if this is not imported directly due to circular | ||
// dependencies | ||
// See https://github.com/expo/expo/issues/35100 | ||
import type { IterableEdgeInsetDetails } from '../../core/types/IterableEdgeInsetDetails'; |
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.
unused expression, expected an assignment or function call
// expo throws an error if this is not imported directly due to circular | ||
// dependencies | ||
// See https://github.com/expo/expo/issues/35100 | ||
import type { IterableInAppContentType } from '../enums/IterableInAppContentType'; |
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.
unused expression, expected an assignment or function call
IterableInAppManager, | ||
IterableInAppMessage, | ||
} from '../../inApp'; | ||
// TODO: Organize these so that there are no circular dependencies |
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.
example/Gemfile
Outdated
@@ -6,3 +6,4 @@ ruby ">= 2.6.10" | |||
# Exclude problematic versions of cocoapods and activesupport that causes build failures. | |||
gem 'cocoapods', '>= 1.13', '!= 1.15.0', '!= 1.15.1' | |||
gem 'activesupport', '>= 6.1.7.5', '!= 7.1.0' | |||
gem 'concurrent-ruby', '< 1.3.4' |
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.
Expected a newline at the end of the file.
example/Gemfile
Outdated
@@ -6,3 +6,4 @@ ruby ">= 2.6.10" | |||
# Exclude problematic versions of cocoapods and activesupport that causes build failures. | |||
gem 'cocoapods', '>= 1.13', '!= 1.15.0', '!= 1.15.1' | |||
gem 'activesupport', '>= 6.1.7.5', '!= 7.1.0' | |||
gem 'concurrent-ruby', '< 1.3.4' |
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.
IterableInAppManager, | ||
IterableInAppMessage, | ||
} from '../../inApp'; | ||
// TODO: Organize these so that there are no circular dependencies |
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.
…ust package version
…hub.com:Iterable/react-native-sdk into expo/MOB-11175-make-rn-sdk-compatible-with-expo * 'expo/MOB-11175-make-rn-sdk-compatible-with-expo' of github.com:Iterable/react-native-sdk: Concurrent Ruby version
…int and TypeScript
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.
Looks good. Only caveat is Xcode 16.2.
Code Climate has analyzed commit bf51fd5 and detected 3 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 71.1% (50% is the threshold). This pull request will bring the total coverage in the repository to 38.4% (0.5% change). View more on Code Climate. |
@@ -6,3 +6,11 @@ ruby ">= 2.6.10" | |||
# Exclude problematic versions of cocoapods and activesupport that causes build failures. | |||
gem 'cocoapods', '>= 1.13', '!= 1.15.0', '!= 1.15.1' | |||
gem 'activesupport', '>= 6.1.7.5', '!= 7.1.0' | |||
gem 'xcodeproj', '< 1.26.0' | |||
gem 'concurrent-ruby', '< 1.3.4' |
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.
# Ruby 3.4.0 has removed some libraries from the standard library. | ||
gem 'bigdecimal' | ||
gem 'logger' | ||
gem 'benchmark' |
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.
IterableInAppManager, | ||
IterableInAppMessage, | ||
} from '../../inApp'; | ||
// TODO: Organize these so that there are no circular dependencies |
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.
🔹 JIRA Ticket(s) if any
✏️ Description
Fixes issues that cause expo builds to fail
Test
Just run the example app and make sure that everything loads as expected. None of the changes are high risk, so a quick smoke check is fine.