-
Notifications
You must be signed in to change notification settings - Fork 13
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
chore: apns sample app upgraded to 0.73.6 #257
base: main
Are you sure you want to change the base?
Conversation
@@ -9,7 +9,7 @@ | |||
"allowUnreachableCode": false, | |||
"allowUnusedLabels": false, | |||
"esModuleInterop": true, | |||
"importsNotUsedAsValues": "error", | |||
"verbatimModuleSyntax": true, |
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.
You will not find this update on FCM app since this change is done at the root directory and adding same to both PRs could possibly cause conflicts.
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 the CI is failing because of this change?
Are you planning on fixing the CI status checks before merging?
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.
Ah! I didn't know that updating this key could break CI. I will check it before merging.
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.
Looking good! This is the last unresolved conversation and it appears the CI status checks are still failing.
I approve of all the changes done thus far. Assuming that more code will need to be changed and then reviewed to get the CI fixed, I will wait to approve.
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.
@levibostian By any chance you know what version of typescript is CI using ? verbatimModuleSyntax
is introduced in Typescript 5 and we seem not be pushing package-lock.json
file for the sample 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.
The CI is failing when it runs yarn on the root directory of the RN SDK, not running yarn in the sample app subdirectories.
I do see that you updated the package.json
file in this PR but the yarn.lock
file didn't get updated. That might be the issue. Updating the yarn.lock file and pushing that up might fix this issue.
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.
See latest comment. Happy to review again when you're ready.
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.
Putting this back in Aman's queue till the mentioned review comments are resolved
part of: https://linear.app/customerio/issue/MBL-203/check-compatibility-of-rn-sdk-with-rn-v73
Updates APN(iOS) sample app with latest (stable) react native version
0.73.6
.In case anyone of you wants to try out the upgrade and are having issues, I have listed down some troubleshoot topics here.