Skip to content

fixed SDK issue while uploading app in debug scheme - #33153

Closed
arinjay wants to merge 5 commits into
react:mainfrom
arinjay:arinjay/iOSDebugTestFlightBugFix
Closed

fixed SDK issue while uploading app in debug scheme#33153
arinjay wants to merge 5 commits into
react:mainfrom
arinjay:arinjay/iOSDebugTestFlightBugFix

Conversation

@arinjay

@arinjayarinjay commented Feb 21, 2022

Copy link
Copy Markdown
Contributor

Summary

Problem - Error when trying to publish to Apple Store in debug scheme

Error thread - #31507

Changelog

[iOS][Changed] - The diffs renames the required variable which was causing conflicts in names with Apple core SDK's

@facebook-github-bot

Copy link
Copy Markdown
Contributor

Hi @arinjay!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@pull-bot

pull-bot commented Feb 21, 2022

Copy link
Copy Markdown
Messages
📖📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.

Generated by 🚫 dangerJS against 6cd06c6

@analysis-bot

analysis-bot commented Feb 21, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
ios-universaln/a--

Base commit: ed46ea2
Branch: main

@analysis-bot

analysis-bot commented Feb 21, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,188,328+415,607
androidhermesarmeabi-v7a7,788,450+610,516
androidhermesx868,558,277+476,782
androidhermesx86_648,511,318+449,549
androidjscarm64-v8a9,857,663+212,091
androidjscarmeabi-v7a8,842,331+422,770
androidjscx869,823,706+228,766
androidjscx86_6410,420,886+228,480

Base commit: ed46ea2
Branch: main

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 21, 2022
@facebook-github-bot

Copy link
Copy Markdown
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@facebook-github-botfacebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Feb 21, 2022
@facebook-github-bot

Copy link
Copy Markdown
Contributor

@SSHIC has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@arinjay

arinjay commented Feb 25, 2022

Copy link
Copy Markdown
ContributorAuthor

@lunaleaps can you help me in getting this MR merged

@kelset

Copy link
Copy Markdown
Contributor

@cortinico & @fortmarek this PR addressed the fact that if you try to upload to Apple's store a debug build, there seems to be an overlap of methods' names with the ones from the core iOS SDK. By changing them, it seems to address the problem - so I think overall it'd be a good change to integrate.
My only concern is that we might have to consider it a breaking change? I don't think we need to merge this for 0.69, but the issue has been open since May 2021 so it'd be good to get this sorted at some point.

(cc @tido64, do you have any opinions on this?)

@react-native-botreact-native-bot added the Platform: iOS iOS applications. label Apr 20, 2022
@fortmarek

Copy link
Copy Markdown
Contributor

My only concern is that we might have to consider it a breaking change?

The properties changed are with _ prefix and thus should be considered private. The only change which could be imho breaking is renaming of handleNotification to handlePackageNotification. Are we sure this is something we consider part of the public API? If so, it should be a breaking change, indeed.

I don't think there's a particular reason not to merge this change for 0.69.

Comment threadReact/Base/RCTKeyCommands.m Outdated
Comment threadReact/DevSupport/RCTPackagerClient.h Outdated
Comment threadReact/DevSupport/RCTPackagerConnection.mm Outdated
arinjayand others added 3 commits May 6, 2022 20:10
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
@cortinico

Copy link
Copy Markdown
Contributor

Looping @cipolleschi in here.

My only concern is that we might have to consider it a breaking change? I don't think we need to merge this for 0.69, but the issue has been open since May 2021 so it'd be good to get this sorted at some point.

IMHO we're fine merging this on main + mark it as a breaking change in the README.

I don't think there's a particular reason not to merge this change for 0.69.

Given the limited use case ("trying to publish to Apple Store in debug scheme") this is not pick-worthy for .69 imho.

@aakashchoubey

aakashchoubey commented May 7, 2022

Copy link
Copy Markdown

Given the limited use case ("trying to publish to Apple Store in debug scheme") this is not pick-worthy for .69 imho.

Just curious about the impact of this breaking change here. From what I understand, since iOS doesn't allow users to install apps as Android does with apks, Testflight is the only available option to upload an iOS build and test for non-ios developers.

Not supporting that is bad dev-exp right?

@cipolleschi

Copy link
Copy Markdown
Contributor

Thanks @cortinico. I agree, it is not pick worthy.

I'm also not so sure about it being breaking: is the RCTPackagerClient class supposed to be accessed and used by clients or should it be an internal details? In other terms, is it a private protocol or not? If the answer is yes, it definitely isn't a breaking change.

@choubeyaakash77, more or less yes, although I don't remember if we can actually submit something in Debug mode nor whether it is a good practice. By the way, the PR has been open since May 2021, as @kelset commented, so it is a year that this is broken. Waiting 1 more release won't hurt more than how it is already doing.

What do you think?

@aakashchoubey

Copy link
Copy Markdown

Waiting 1 more release won't hurt more than how it is already doing.

@cipolleschi Yeah it makes sense. If it is not a breaking change, there should be no problem in including it in 0.69, but if there is a breaking change, then that's a call on the react-native team I guess.

submit something in Debug mode nor whether it is a good practice

Submitting on TestFlight is different from submitting on AppStore. I am not sure but I don't think submitting something in Debug mode is a bad practice. That's a limitation given to us by Apple.
There is a valid use case imo to float around an app of a different variant than Release, to some internal users. There are many reasons for doing that - an app for backend developers, adding specific options for variants that won't be available in a release app, etc. One can potentially achieve the same by maintaining different branches/tags but that brings an overhead of maintaining multiple codebases. And this is fairly common in Android to make things available by app's variant.

@cipolleschi

Copy link
Copy Markdown
Contributor

Yeah, I see all the points and I agree with them.

What I was trying to say, failing, is that, if I remember well:

  • submitting an app to the App Store is one thing, and IIRC it must be built for release.
  • submitting an app for TestFlight should follow a different path and it can be sent in Debug mode.

So these warnings looks a little bit weird to me.

But again, it has a been a while since I had to submit something to Apple, and I was using an Enterprise profile to distribute the app internally in other context. So, I could remember wrong.

@cortinico

Copy link
Copy Markdown
Contributor

@arinjay Don't you mind updating the changelog entry to mention this is a breaking change?

@arinjay

Copy link
Copy Markdown
ContributorAuthor

@cortinico I can't see how it's a breaking change?

As per my understanding of the code, RCTPackagerClient is supposed to be an internal protocol used by SDK itself and not directly by the client.
So updating the method definition both at the protocol and implementation level can't break it on the client-side.

@arinjay

Copy link
Copy Markdown
ContributorAuthor

@cipolleschi yes you’re right

  1. submitting an app to the App Store (release mode) in one thing
  2. submitting an app for TestFlight(debug mode) is separate.

Schemes are different on iOS apps.

The need comes from the very limitation Apple has in the first place i.e. not being able to install IPA directly onto any iOS device.
This generally hampers the development cycle and the majority of folks have to resolve to release apps in debug mode on a test flight as not all end users have machines with Xcode.

Sharing it with multiple end-users and then gathering bugs becomes very easier. The whole iOS development cycle becomes more efficient.

@Tak-As

Copy link
Copy Markdown

When will this PR be merged?
I am facing this issue and am waiting for a merge.
I appreciate your help with moving this issue ahead.

@cortinico

Copy link
Copy Markdown
Contributor

When will this PR be merged?
I am facing this issue and am waiting for a merge.
I appreciate your help with moving this issue ahead.

Just to clarify, this won't be included in RN 0.69. The earliest this will be released in RN 0.70 as it's not considered a regression.

@react-native-bot

Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @arinjay in 086c13d.

When will my fix make it into a release? | Upcoming Releases

@react-native-botreact-native-bot added the Merged This PR has been merged. label Jun 6, 2022
@javache

Copy link
Copy Markdown
Contributor

I don't believe this is correct to fix and will revert this. This code is internal only and therefore can use private API's. You should never upload debug scheme builds to the App Store.

@javache

Copy link
Copy Markdown
Contributor

@cipolleschi and I look at an alternative way to address the issue in #31507. For now this breaks some development workflows, so we prefer to back this out.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request has been reverted by 6fcb878.

@arinjay

Copy link
Copy Markdown
ContributorAuthor

@javache what's the alternate way of resolving this? Can you mention that

Also "You should never upload debug scheme builds to the App Store"
This is correct
but we didn't upload it to AppStore, it failed to upload on the testFlight (these 2 are different mediums to publish app), even without the above MR changes.

Can you mention the changes using which we can still upload the app on testflight in debug mode?

@javache

Copy link
Copy Markdown
Contributor

You should never upload debug scheme builds to any distribution platform. They are inherently insecure as they allow any local packager instance to completely redefine your application's behaviour, while still providing access to privileged local storage (eg keychain).

@arinjay

Copy link
Copy Markdown
ContributorAuthor

'You should never upload debug scheme builds to any distribution platform'
Why is that?
Apple made testfight for this purpose only. People to whom the app is made available are aware of the access they will be sharing when downloading the app from testflight. Apple made sure to list out all potential risks before setting up test flight apps.

Use case in which debug mode is required - An app running both on react native and native (swift) codebase. now in debug mode we need to have one API call debugger library or profiler library which we only want in debug mode only, and installed dependencies in debug mode only. Post putting it on test flight to share it with other user, which in real-time can track the profiler data on actual devices or detect which API was fired.

These cases are very much required in many day-to-day app developments.

@gabriellend

gabriellend commented Aug 29, 2022

Copy link
Copy Markdown

@javache@arinjay Hi! Not sure this is still an ongoing conversation as the last reply was in June but I don't think it was implemented as I don't see it in the tags. I wanted to throw my two cents in as I can attest to the importance of being able to upload a debug scheme to TestFlight for internal testing before we submit a release version to the App Store. We've been doing it this way for years at my job (we have a staging environment and a production environment, so the debug scheme works off of staging and the release scheme works off production) and we now can't submit to TestFlight because of running into the selectors mentioned in this post. I've been in talks with Apple and they just got back to me directing me to manually change these myself so that the upload will be accepted to TestFlight. I want to voice my support for including this in the package. Thanks!

@rolfen

rolfen commented Mar 12, 2024

Copy link
Copy Markdown

@javache Whether you should upload debug scheme builds to the App Store or not is outside RN's scope.

This error message suggests an issue, either in the RN code or in Apple's validation.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.Platform: iOSiOS applications.RevertedShared with MetaApplied via automation to indicate that an Issue or Pull Request has been shared with the team.Type: Removal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

15 participants

@arinjay@facebook-github-bot@pull-bot@analysis-bot@kelset@fortmarek@cortinico@aakashchoubey@cipolleschi@Tak-As@react-native-bot@javache@gabriellend@rolfen@tido64
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
fixed SDK issue while uploading app in debug scheme by arinjay · Pull Request #33153 · react/react-native · GitHub
Skip to content

fixed SDK issue while uploading app in debug scheme - #33153

Closed
arinjay wants to merge 5 commits into
react:mainfrom
arinjay:arinjay/iOSDebugTestFlightBugFix
Closed

fixed SDK issue while uploading app in debug scheme#33153
arinjay wants to merge 5 commits into
react:mainfrom
arinjay:arinjay/iOSDebugTestFlightBugFix

Conversation

@arinjay

@arinjayarinjay commented Feb 21, 2022

Copy link
Copy Markdown
Contributor

Summary

Problem - Error when trying to publish to Apple Store in debug scheme

Error thread - #31507

Changelog

[iOS][Changed] - The diffs renames the required variable which was causing conflicts in names with Apple core SDK's

@facebook-github-bot

Copy link
Copy Markdown
Contributor

Hi @arinjay!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@pull-bot

pull-bot commented Feb 21, 2022

Copy link
Copy Markdown
Messages
📖📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.

Generated by 🚫 dangerJS against 6cd06c6

@analysis-bot

analysis-bot commented Feb 21, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
ios-universaln/a--

Base commit: ed46ea2
Branch: main

@analysis-bot

analysis-bot commented Feb 21, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,188,328+415,607
androidhermesarmeabi-v7a7,788,450+610,516
androidhermesx868,558,277+476,782
androidhermesx86_648,511,318+449,549
androidjscarm64-v8a9,857,663+212,091
androidjscarmeabi-v7a8,842,331+422,770
androidjscx869,823,706+228,766
androidjscx86_6410,420,886+228,480

Base commit: ed46ea2
Branch: main

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 21, 2022
@facebook-github-bot

Copy link
Copy Markdown
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@facebook-github-botfacebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Feb 21, 2022
@facebook-github-bot

Copy link
Copy Markdown
Contributor

@SSHIC has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@arinjay

arinjay commented Feb 25, 2022

Copy link
Copy Markdown
ContributorAuthor

@lunaleaps can you help me in getting this MR merged

@kelset

Copy link
Copy Markdown
Contributor

@cortinico & @fortmarek this PR addressed the fact that if you try to upload to Apple's store a debug build, there seems to be an overlap of methods' names with the ones from the core iOS SDK. By changing them, it seems to address the problem - so I think overall it'd be a good change to integrate.
My only concern is that we might have to consider it a breaking change? I don't think we need to merge this for 0.69, but the issue has been open since May 2021 so it'd be good to get this sorted at some point.

(cc @tido64, do you have any opinions on this?)

@react-native-botreact-native-bot added the Platform: iOS iOS applications. label Apr 20, 2022
@fortmarek

Copy link
Copy Markdown
Contributor

My only concern is that we might have to consider it a breaking change?

The properties changed are with _ prefix and thus should be considered private. The only change which could be imho breaking is renaming of handleNotification to handlePackageNotification. Are we sure this is something we consider part of the public API? If so, it should be a breaking change, indeed.

I don't think there's a particular reason not to merge this change for 0.69.

Comment threadReact/Base/RCTKeyCommands.m Outdated
Comment threadReact/DevSupport/RCTPackagerClient.h Outdated
Comment threadReact/DevSupport/RCTPackagerConnection.mm Outdated
arinjayand others added 3 commits May 6, 2022 20:10
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
@cortinico

Copy link
Copy Markdown
Contributor

Looping @cipolleschi in here.

My only concern is that we might have to consider it a breaking change? I don't think we need to merge this for 0.69, but the issue has been open since May 2021 so it'd be good to get this sorted at some point.

IMHO we're fine merging this on main + mark it as a breaking change in the README.

I don't think there's a particular reason not to merge this change for 0.69.

Given the limited use case ("trying to publish to Apple Store in debug scheme") this is not pick-worthy for .69 imho.

@aakashchoubey

aakashchoubey commented May 7, 2022

Copy link
Copy Markdown

Given the limited use case ("trying to publish to Apple Store in debug scheme") this is not pick-worthy for .69 imho.

Just curious about the impact of this breaking change here. From what I understand, since iOS doesn't allow users to install apps as Android does with apks, Testflight is the only available option to upload an iOS build and test for non-ios developers.

Not supporting that is bad dev-exp right?

@cipolleschi

Copy link
Copy Markdown
Contributor

Thanks @cortinico. I agree, it is not pick worthy.

I'm also not so sure about it being breaking: is the RCTPackagerClient class supposed to be accessed and used by clients or should it be an internal details? In other terms, is it a private protocol or not? If the answer is yes, it definitely isn't a breaking change.

@choubeyaakash77, more or less yes, although I don't remember if we can actually submit something in Debug mode nor whether it is a good practice. By the way, the PR has been open since May 2021, as @kelset commented, so it is a year that this is broken. Waiting 1 more release won't hurt more than how it is already doing.

What do you think?

@aakashchoubey

Copy link
Copy Markdown

Waiting 1 more release won't hurt more than how it is already doing.

@cipolleschi Yeah it makes sense. If it is not a breaking change, there should be no problem in including it in 0.69, but if there is a breaking change, then that's a call on the react-native team I guess.

submit something in Debug mode nor whether it is a good practice

Submitting on TestFlight is different from submitting on AppStore. I am not sure but I don't think submitting something in Debug mode is a bad practice. That's a limitation given to us by Apple.
There is a valid use case imo to float around an app of a different variant than Release, to some internal users. There are many reasons for doing that - an app for backend developers, adding specific options for variants that won't be available in a release app, etc. One can potentially achieve the same by maintaining different branches/tags but that brings an overhead of maintaining multiple codebases. And this is fairly common in Android to make things available by app's variant.

@cipolleschi

Copy link
Copy Markdown
Contributor

Yeah, I see all the points and I agree with them.

What I was trying to say, failing, is that, if I remember well:

  • submitting an app to the App Store is one thing, and IIRC it must be built for release.
  • submitting an app for TestFlight should follow a different path and it can be sent in Debug mode.

So these warnings looks a little bit weird to me.

But again, it has a been a while since I had to submit something to Apple, and I was using an Enterprise profile to distribute the app internally in other context. So, I could remember wrong.

@cortinico

Copy link
Copy Markdown
Contributor

@arinjay Don't you mind updating the changelog entry to mention this is a breaking change?

@arinjay

Copy link
Copy Markdown
ContributorAuthor

@cortinico I can't see how it's a breaking change?

As per my understanding of the code, RCTPackagerClient is supposed to be an internal protocol used by SDK itself and not directly by the client.
So updating the method definition both at the protocol and implementation level can't break it on the client-side.

@arinjay

Copy link
Copy Markdown
ContributorAuthor

@cipolleschi yes you’re right

  1. submitting an app to the App Store (release mode) in one thing
  2. submitting an app for TestFlight(debug mode) is separate.

Schemes are different on iOS apps.

The need comes from the very limitation Apple has in the first place i.e. not being able to install IPA directly onto any iOS device.
This generally hampers the development cycle and the majority of folks have to resolve to release apps in debug mode on a test flight as not all end users have machines with Xcode.

Sharing it with multiple end-users and then gathering bugs becomes very easier. The whole iOS development cycle becomes more efficient.

@Tak-As

Copy link
Copy Markdown

When will this PR be merged?
I am facing this issue and am waiting for a merge.
I appreciate your help with moving this issue ahead.

@cortinico

Copy link
Copy Markdown
Contributor

When will this PR be merged?
I am facing this issue and am waiting for a merge.
I appreciate your help with moving this issue ahead.

Just to clarify, this won't be included in RN 0.69. The earliest this will be released in RN 0.70 as it's not considered a regression.

@react-native-bot

Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @arinjay in 086c13d.

When will my fix make it into a release? | Upcoming Releases

@react-native-botreact-native-bot added the Merged This PR has been merged. label Jun 6, 2022
@javache

Copy link
Copy Markdown
Contributor

I don't believe this is correct to fix and will revert this. This code is internal only and therefore can use private API's. You should never upload debug scheme builds to the App Store.

@javache

Copy link
Copy Markdown
Contributor

@cipolleschi and I look at an alternative way to address the issue in #31507. For now this breaks some development workflows, so we prefer to back this out.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request has been reverted by 6fcb878.

@arinjay

Copy link
Copy Markdown
ContributorAuthor

@javache what's the alternate way of resolving this? Can you mention that

Also "You should never upload debug scheme builds to the App Store"
This is correct
but we didn't upload it to AppStore, it failed to upload on the testFlight (these 2 are different mediums to publish app), even without the above MR changes.

Can you mention the changes using which we can still upload the app on testflight in debug mode?

@javache

Copy link
Copy Markdown
Contributor

You should never upload debug scheme builds to any distribution platform. They are inherently insecure as they allow any local packager instance to completely redefine your application's behaviour, while still providing access to privileged local storage (eg keychain).

@arinjay

Copy link
Copy Markdown
ContributorAuthor

'You should never upload debug scheme builds to any distribution platform'
Why is that?
Apple made testfight for this purpose only. People to whom the app is made available are aware of the access they will be sharing when downloading the app from testflight. Apple made sure to list out all potential risks before setting up test flight apps.

Use case in which debug mode is required - An app running both on react native and native (swift) codebase. now in debug mode we need to have one API call debugger library or profiler library which we only want in debug mode only, and installed dependencies in debug mode only. Post putting it on test flight to share it with other user, which in real-time can track the profiler data on actual devices or detect which API was fired.

These cases are very much required in many day-to-day app developments.

@gabriellend

gabriellend commented Aug 29, 2022

Copy link
Copy Markdown

@javache@arinjay Hi! Not sure this is still an ongoing conversation as the last reply was in June but I don't think it was implemented as I don't see it in the tags. I wanted to throw my two cents in as I can attest to the importance of being able to upload a debug scheme to TestFlight for internal testing before we submit a release version to the App Store. We've been doing it this way for years at my job (we have a staging environment and a production environment, so the debug scheme works off of staging and the release scheme works off production) and we now can't submit to TestFlight because of running into the selectors mentioned in this post. I've been in talks with Apple and they just got back to me directing me to manually change these myself so that the upload will be accepted to TestFlight. I want to voice my support for including this in the package. Thanks!

@rolfen

rolfen commented Mar 12, 2024

Copy link
Copy Markdown

@javache Whether you should upload debug scheme builds to the App Store or not is outside RN's scope.

This error message suggests an issue, either in the RN code or in Apple's validation.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.Platform: iOSiOS applications.RevertedShared with MetaApplied via automation to indicate that an Issue or Pull Request has been shared with the team.Type: Removal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

15 participants

@arinjay@facebook-github-bot@pull-bot@analysis-bot@kelset@fortmarek@cortinico@aakashchoubey@cipolleschi@Tak-As@react-native-bot@javache@gabriellend@rolfen@tido64
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fixed SDK issue while uploading app in debug scheme by arinjay · Pull Request #33153 · react/react-native · GitHub
Skip to content

fixed SDK issue while uploading app in debug scheme - #33153

Closed
arinjay wants to merge 5 commits into
react:mainfrom
arinjay:arinjay/iOSDebugTestFlightBugFix
Closed

fixed SDK issue while uploading app in debug scheme#33153
arinjay wants to merge 5 commits into
react:mainfrom
arinjay:arinjay/iOSDebugTestFlightBugFix

Conversation

@arinjay

@arinjayarinjay commented Feb 21, 2022

Copy link
Copy Markdown
Contributor

Summary

Problem - Error when trying to publish to Apple Store in debug scheme

Error thread - #31507

Changelog

[iOS][Changed] - The diffs renames the required variable which was causing conflicts in names with Apple core SDK's

@facebook-github-bot

Copy link
Copy Markdown
Contributor

Hi @arinjay!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@pull-bot

pull-bot commented Feb 21, 2022

Copy link
Copy Markdown
Messages
📖📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.

Generated by 🚫 dangerJS against 6cd06c6

@analysis-bot

analysis-bot commented Feb 21, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
ios-universaln/a--

Base commit: ed46ea2
Branch: main

@analysis-bot

analysis-bot commented Feb 21, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,188,328+415,607
androidhermesarmeabi-v7a7,788,450+610,516
androidhermesx868,558,277+476,782
androidhermesx86_648,511,318+449,549
androidjscarm64-v8a9,857,663+212,091
androidjscarmeabi-v7a8,842,331+422,770
androidjscx869,823,706+228,766
androidjscx86_6410,420,886+228,480

Base commit: ed46ea2
Branch: main

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 21, 2022
@facebook-github-bot

Copy link
Copy Markdown
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@facebook-github-botfacebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Feb 21, 2022
@facebook-github-bot

Copy link
Copy Markdown
Contributor

@SSHIC has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@arinjay

arinjay commented Feb 25, 2022

Copy link
Copy Markdown
ContributorAuthor

@lunaleaps can you help me in getting this MR merged

@kelset

Copy link
Copy Markdown
Contributor

@cortinico & @fortmarek this PR addressed the fact that if you try to upload to Apple's store a debug build, there seems to be an overlap of methods' names with the ones from the core iOS SDK. By changing them, it seems to address the problem - so I think overall it'd be a good change to integrate.
My only concern is that we might have to consider it a breaking change? I don't think we need to merge this for 0.69, but the issue has been open since May 2021 so it'd be good to get this sorted at some point.

(cc @tido64, do you have any opinions on this?)

@react-native-botreact-native-bot added the Platform: iOS iOS applications. label Apr 20, 2022
@fortmarek

Copy link
Copy Markdown
Contributor

My only concern is that we might have to consider it a breaking change?

The properties changed are with _ prefix and thus should be considered private. The only change which could be imho breaking is renaming of handleNotification to handlePackageNotification. Are we sure this is something we consider part of the public API? If so, it should be a breaking change, indeed.

I don't think there's a particular reason not to merge this change for 0.69.

Comment threadReact/Base/RCTKeyCommands.m Outdated
Comment threadReact/DevSupport/RCTPackagerClient.h Outdated
Comment threadReact/DevSupport/RCTPackagerConnection.mm Outdated
arinjayand others added 3 commits May 6, 2022 20:10
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
@cortinico

Copy link
Copy Markdown
Contributor

Looping @cipolleschi in here.

My only concern is that we might have to consider it a breaking change? I don't think we need to merge this for 0.69, but the issue has been open since May 2021 so it'd be good to get this sorted at some point.

IMHO we're fine merging this on main + mark it as a breaking change in the README.

I don't think there's a particular reason not to merge this change for 0.69.

Given the limited use case ("trying to publish to Apple Store in debug scheme") this is not pick-worthy for .69 imho.

@aakashchoubey

aakashchoubey commented May 7, 2022

Copy link
Copy Markdown

Given the limited use case ("trying to publish to Apple Store in debug scheme") this is not pick-worthy for .69 imho.

Just curious about the impact of this breaking change here. From what I understand, since iOS doesn't allow users to install apps as Android does with apks, Testflight is the only available option to upload an iOS build and test for non-ios developers.

Not supporting that is bad dev-exp right?

@cipolleschi

Copy link
Copy Markdown
Contributor

Thanks @cortinico. I agree, it is not pick worthy.

I'm also not so sure about it being breaking: is the RCTPackagerClient class supposed to be accessed and used by clients or should it be an internal details? In other terms, is it a private protocol or not? If the answer is yes, it definitely isn't a breaking change.

@choubeyaakash77, more or less yes, although I don't remember if we can actually submit something in Debug mode nor whether it is a good practice. By the way, the PR has been open since May 2021, as @kelset commented, so it is a year that this is broken. Waiting 1 more release won't hurt more than how it is already doing.

What do you think?

@aakashchoubey

Copy link
Copy Markdown

Waiting 1 more release won't hurt more than how it is already doing.

@cipolleschi Yeah it makes sense. If it is not a breaking change, there should be no problem in including it in 0.69, but if there is a breaking change, then that's a call on the react-native team I guess.

submit something in Debug mode nor whether it is a good practice

Submitting on TestFlight is different from submitting on AppStore. I am not sure but I don't think submitting something in Debug mode is a bad practice. That's a limitation given to us by Apple.
There is a valid use case imo to float around an app of a different variant than Release, to some internal users. There are many reasons for doing that - an app for backend developers, adding specific options for variants that won't be available in a release app, etc. One can potentially achieve the same by maintaining different branches/tags but that brings an overhead of maintaining multiple codebases. And this is fairly common in Android to make things available by app's variant.

@cipolleschi

Copy link
Copy Markdown
Contributor

Yeah, I see all the points and I agree with them.

What I was trying to say, failing, is that, if I remember well:

  • submitting an app to the App Store is one thing, and IIRC it must be built for release.
  • submitting an app for TestFlight should follow a different path and it can be sent in Debug mode.

So these warnings looks a little bit weird to me.

But again, it has a been a while since I had to submit something to Apple, and I was using an Enterprise profile to distribute the app internally in other context. So, I could remember wrong.

@cortinico

Copy link
Copy Markdown
Contributor

@arinjay Don't you mind updating the changelog entry to mention this is a breaking change?

@arinjay

Copy link
Copy Markdown
ContributorAuthor

@cortinico I can't see how it's a breaking change?

As per my understanding of the code, RCTPackagerClient is supposed to be an internal protocol used by SDK itself and not directly by the client.
So updating the method definition both at the protocol and implementation level can't break it on the client-side.

@arinjay

Copy link
Copy Markdown
ContributorAuthor

@cipolleschi yes you’re right

  1. submitting an app to the App Store (release mode) in one thing
  2. submitting an app for TestFlight(debug mode) is separate.

Schemes are different on iOS apps.

The need comes from the very limitation Apple has in the first place i.e. not being able to install IPA directly onto any iOS device.
This generally hampers the development cycle and the majority of folks have to resolve to release apps in debug mode on a test flight as not all end users have machines with Xcode.

Sharing it with multiple end-users and then gathering bugs becomes very easier. The whole iOS development cycle becomes more efficient.

@Tak-As

Copy link
Copy Markdown

When will this PR be merged?
I am facing this issue and am waiting for a merge.
I appreciate your help with moving this issue ahead.

@cortinico

Copy link
Copy Markdown
Contributor

When will this PR be merged?
I am facing this issue and am waiting for a merge.
I appreciate your help with moving this issue ahead.

Just to clarify, this won't be included in RN 0.69. The earliest this will be released in RN 0.70 as it's not considered a regression.

@react-native-bot

Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @arinjay in 086c13d.

When will my fix make it into a release? | Upcoming Releases

@react-native-botreact-native-bot added the Merged This PR has been merged. label Jun 6, 2022
@javache

Copy link
Copy Markdown
Contributor

I don't believe this is correct to fix and will revert this. This code is internal only and therefore can use private API's. You should never upload debug scheme builds to the App Store.

@javache

Copy link
Copy Markdown
Contributor

@cipolleschi and I look at an alternative way to address the issue in #31507. For now this breaks some development workflows, so we prefer to back this out.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request has been reverted by 6fcb878.

@arinjay

Copy link
Copy Markdown
ContributorAuthor

@javache what's the alternate way of resolving this? Can you mention that

Also "You should never upload debug scheme builds to the App Store"
This is correct
but we didn't upload it to AppStore, it failed to upload on the testFlight (these 2 are different mediums to publish app), even without the above MR changes.

Can you mention the changes using which we can still upload the app on testflight in debug mode?

@javache

Copy link
Copy Markdown
Contributor

You should never upload debug scheme builds to any distribution platform. They are inherently insecure as they allow any local packager instance to completely redefine your application's behaviour, while still providing access to privileged local storage (eg keychain).

@arinjay

Copy link
Copy Markdown
ContributorAuthor

'You should never upload debug scheme builds to any distribution platform'
Why is that?
Apple made testfight for this purpose only. People to whom the app is made available are aware of the access they will be sharing when downloading the app from testflight. Apple made sure to list out all potential risks before setting up test flight apps.

Use case in which debug mode is required - An app running both on react native and native (swift) codebase. now in debug mode we need to have one API call debugger library or profiler library which we only want in debug mode only, and installed dependencies in debug mode only. Post putting it on test flight to share it with other user, which in real-time can track the profiler data on actual devices or detect which API was fired.

These cases are very much required in many day-to-day app developments.

@gabriellend

gabriellend commented Aug 29, 2022

Copy link
Copy Markdown

@javache@arinjay Hi! Not sure this is still an ongoing conversation as the last reply was in June but I don't think it was implemented as I don't see it in the tags. I wanted to throw my two cents in as I can attest to the importance of being able to upload a debug scheme to TestFlight for internal testing before we submit a release version to the App Store. We've been doing it this way for years at my job (we have a staging environment and a production environment, so the debug scheme works off of staging and the release scheme works off production) and we now can't submit to TestFlight because of running into the selectors mentioned in this post. I've been in talks with Apple and they just got back to me directing me to manually change these myself so that the upload will be accepted to TestFlight. I want to voice my support for including this in the package. Thanks!

@rolfen

rolfen commented Mar 12, 2024

Copy link
Copy Markdown

@javache Whether you should upload debug scheme builds to the App Store or not is outside RN's scope.

This error message suggests an issue, either in the RN code or in Apple's validation.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.Platform: iOSiOS applications.RevertedShared with MetaApplied via automation to indicate that an Issue or Pull Request has been shared with the team.Type: Removal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

15 participants

@arinjay@facebook-github-bot@pull-bot@analysis-bot@kelset@fortmarek@cortinico@aakashchoubey@cipolleschi@Tak-As@react-native-bot@javache@gabriellend@rolfen@tido64
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fixed SDK issue while uploading app in debug scheme by arinjay · Pull Request #33153 · react/react-native · GitHub
Skip to content

fixed SDK issue while uploading app in debug scheme - #33153

Closed
arinjay wants to merge 5 commits into
react:mainfrom
arinjay:arinjay/iOSDebugTestFlightBugFix
Closed

fixed SDK issue while uploading app in debug scheme#33153
arinjay wants to merge 5 commits into
react:mainfrom
arinjay:arinjay/iOSDebugTestFlightBugFix

Conversation

@arinjay

@arinjayarinjay commented Feb 21, 2022

Copy link
Copy Markdown
Contributor

Summary

Problem - Error when trying to publish to Apple Store in debug scheme

Error thread - #31507

Changelog

[iOS][Changed] - The diffs renames the required variable which was causing conflicts in names with Apple core SDK's

@facebook-github-bot

Copy link
Copy Markdown
Contributor

Hi @arinjay!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@pull-bot

pull-bot commented Feb 21, 2022

Copy link
Copy Markdown
Messages
📖📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.

Generated by 🚫 dangerJS against 6cd06c6

@analysis-bot

analysis-bot commented Feb 21, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
ios-universaln/a--

Base commit: ed46ea2
Branch: main

@analysis-bot

analysis-bot commented Feb 21, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,188,328+415,607
androidhermesarmeabi-v7a7,788,450+610,516
androidhermesx868,558,277+476,782
androidhermesx86_648,511,318+449,549
androidjscarm64-v8a9,857,663+212,091
androidjscarmeabi-v7a8,842,331+422,770
androidjscx869,823,706+228,766
androidjscx86_6410,420,886+228,480

Base commit: ed46ea2
Branch: main

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 21, 2022
@facebook-github-bot

Copy link
Copy Markdown
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@facebook-github-botfacebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Feb 21, 2022
@facebook-github-bot

Copy link
Copy Markdown
Contributor

@SSHIC has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@arinjay

arinjay commented Feb 25, 2022

Copy link
Copy Markdown
ContributorAuthor

@lunaleaps can you help me in getting this MR merged

@kelset

Copy link
Copy Markdown
Contributor

@cortinico & @fortmarek this PR addressed the fact that if you try to upload to Apple's store a debug build, there seems to be an overlap of methods' names with the ones from the core iOS SDK. By changing them, it seems to address the problem - so I think overall it'd be a good change to integrate.
My only concern is that we might have to consider it a breaking change? I don't think we need to merge this for 0.69, but the issue has been open since May 2021 so it'd be good to get this sorted at some point.

(cc @tido64, do you have any opinions on this?)

@react-native-botreact-native-bot added the Platform: iOS iOS applications. label Apr 20, 2022
@fortmarek

Copy link
Copy Markdown
Contributor

My only concern is that we might have to consider it a breaking change?

The properties changed are with _ prefix and thus should be considered private. The only change which could be imho breaking is renaming of handleNotification to handlePackageNotification. Are we sure this is something we consider part of the public API? If so, it should be a breaking change, indeed.

I don't think there's a particular reason not to merge this change for 0.69.

Comment threadReact/Base/RCTKeyCommands.m Outdated
Comment threadReact/DevSupport/RCTPackagerClient.h Outdated
Comment threadReact/DevSupport/RCTPackagerConnection.mm Outdated
arinjayand others added 3 commits May 6, 2022 20:10
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
@cortinico

Copy link
Copy Markdown
Contributor

Looping @cipolleschi in here.

My only concern is that we might have to consider it a breaking change? I don't think we need to merge this for 0.69, but the issue has been open since May 2021 so it'd be good to get this sorted at some point.

IMHO we're fine merging this on main + mark it as a breaking change in the README.

I don't think there's a particular reason not to merge this change for 0.69.

Given the limited use case ("trying to publish to Apple Store in debug scheme") this is not pick-worthy for .69 imho.

@aakashchoubey

aakashchoubey commented May 7, 2022

Copy link
Copy Markdown

Given the limited use case ("trying to publish to Apple Store in debug scheme") this is not pick-worthy for .69 imho.

Just curious about the impact of this breaking change here. From what I understand, since iOS doesn't allow users to install apps as Android does with apks, Testflight is the only available option to upload an iOS build and test for non-ios developers.

Not supporting that is bad dev-exp right?

@cipolleschi

Copy link
Copy Markdown
Contributor

Thanks @cortinico. I agree, it is not pick worthy.

I'm also not so sure about it being breaking: is the RCTPackagerClient class supposed to be accessed and used by clients or should it be an internal details? In other terms, is it a private protocol or not? If the answer is yes, it definitely isn't a breaking change.

@choubeyaakash77, more or less yes, although I don't remember if we can actually submit something in Debug mode nor whether it is a good practice. By the way, the PR has been open since May 2021, as @kelset commented, so it is a year that this is broken. Waiting 1 more release won't hurt more than how it is already doing.

What do you think?

@aakashchoubey

Copy link
Copy Markdown

Waiting 1 more release won't hurt more than how it is already doing.

@cipolleschi Yeah it makes sense. If it is not a breaking change, there should be no problem in including it in 0.69, but if there is a breaking change, then that's a call on the react-native team I guess.

submit something in Debug mode nor whether it is a good practice

Submitting on TestFlight is different from submitting on AppStore. I am not sure but I don't think submitting something in Debug mode is a bad practice. That's a limitation given to us by Apple.
There is a valid use case imo to float around an app of a different variant than Release, to some internal users. There are many reasons for doing that - an app for backend developers, adding specific options for variants that won't be available in a release app, etc. One can potentially achieve the same by maintaining different branches/tags but that brings an overhead of maintaining multiple codebases. And this is fairly common in Android to make things available by app's variant.

@cipolleschi

Copy link
Copy Markdown
Contributor

Yeah, I see all the points and I agree with them.

What I was trying to say, failing, is that, if I remember well:

  • submitting an app to the App Store is one thing, and IIRC it must be built for release.
  • submitting an app for TestFlight should follow a different path and it can be sent in Debug mode.

So these warnings looks a little bit weird to me.

But again, it has a been a while since I had to submit something to Apple, and I was using an Enterprise profile to distribute the app internally in other context. So, I could remember wrong.

@cortinico

Copy link
Copy Markdown
Contributor

@arinjay Don't you mind updating the changelog entry to mention this is a breaking change?

@arinjay

Copy link
Copy Markdown
ContributorAuthor

@cortinico I can't see how it's a breaking change?

As per my understanding of the code, RCTPackagerClient is supposed to be an internal protocol used by SDK itself and not directly by the client.
So updating the method definition both at the protocol and implementation level can't break it on the client-side.

@arinjay

Copy link
Copy Markdown
ContributorAuthor

@cipolleschi yes you’re right

  1. submitting an app to the App Store (release mode) in one thing
  2. submitting an app for TestFlight(debug mode) is separate.

Schemes are different on iOS apps.

The need comes from the very limitation Apple has in the first place i.e. not being able to install IPA directly onto any iOS device.
This generally hampers the development cycle and the majority of folks have to resolve to release apps in debug mode on a test flight as not all end users have machines with Xcode.

Sharing it with multiple end-users and then gathering bugs becomes very easier. The whole iOS development cycle becomes more efficient.

@Tak-As

Copy link
Copy Markdown

When will this PR be merged?
I am facing this issue and am waiting for a merge.
I appreciate your help with moving this issue ahead.

@cortinico

Copy link
Copy Markdown
Contributor

When will this PR be merged?
I am facing this issue and am waiting for a merge.
I appreciate your help with moving this issue ahead.

Just to clarify, this won't be included in RN 0.69. The earliest this will be released in RN 0.70 as it's not considered a regression.

@react-native-bot

Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @arinjay in 086c13d.

When will my fix make it into a release? | Upcoming Releases

@react-native-botreact-native-bot added the Merged This PR has been merged. label Jun 6, 2022
@javache

Copy link
Copy Markdown
Contributor

I don't believe this is correct to fix and will revert this. This code is internal only and therefore can use private API's. You should never upload debug scheme builds to the App Store.

@javache

Copy link
Copy Markdown
Contributor

@cipolleschi and I look at an alternative way to address the issue in #31507. For now this breaks some development workflows, so we prefer to back this out.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request has been reverted by 6fcb878.

@arinjay

Copy link
Copy Markdown
ContributorAuthor

@javache what's the alternate way of resolving this? Can you mention that

Also "You should never upload debug scheme builds to the App Store"
This is correct
but we didn't upload it to AppStore, it failed to upload on the testFlight (these 2 are different mediums to publish app), even without the above MR changes.

Can you mention the changes using which we can still upload the app on testflight in debug mode?

@javache

Copy link
Copy Markdown
Contributor

You should never upload debug scheme builds to any distribution platform. They are inherently insecure as they allow any local packager instance to completely redefine your application's behaviour, while still providing access to privileged local storage (eg keychain).

@arinjay

Copy link
Copy Markdown
ContributorAuthor

'You should never upload debug scheme builds to any distribution platform'
Why is that?
Apple made testfight for this purpose only. People to whom the app is made available are aware of the access they will be sharing when downloading the app from testflight. Apple made sure to list out all potential risks before setting up test flight apps.

Use case in which debug mode is required - An app running both on react native and native (swift) codebase. now in debug mode we need to have one API call debugger library or profiler library which we only want in debug mode only, and installed dependencies in debug mode only. Post putting it on test flight to share it with other user, which in real-time can track the profiler data on actual devices or detect which API was fired.

These cases are very much required in many day-to-day app developments.

@gabriellend

gabriellend commented Aug 29, 2022

Copy link
Copy Markdown

@javache@arinjay Hi! Not sure this is still an ongoing conversation as the last reply was in June but I don't think it was implemented as I don't see it in the tags. I wanted to throw my two cents in as I can attest to the importance of being able to upload a debug scheme to TestFlight for internal testing before we submit a release version to the App Store. We've been doing it this way for years at my job (we have a staging environment and a production environment, so the debug scheme works off of staging and the release scheme works off production) and we now can't submit to TestFlight because of running into the selectors mentioned in this post. I've been in talks with Apple and they just got back to me directing me to manually change these myself so that the upload will be accepted to TestFlight. I want to voice my support for including this in the package. Thanks!

@rolfen

rolfen commented Mar 12, 2024

Copy link
Copy Markdown

@javache Whether you should upload debug scheme builds to the App Store or not is outside RN's scope.

This error message suggests an issue, either in the RN code or in Apple's validation.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.Platform: iOSiOS applications.RevertedShared with MetaApplied via automation to indicate that an Issue or Pull Request has been shared with the team.Type: Removal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

15 participants

@arinjay@facebook-github-bot@pull-bot@analysis-bot@kelset@fortmarek@cortinico@aakashchoubey@cipolleschi@Tak-As@react-native-bot@javache@gabriellend@rolfen@tido64
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' fixed SDK issue while uploading app in debug scheme by arinjay · Pull Request #33153 · react/react-native · GitHub
Skip to content

fixed SDK issue while uploading app in debug scheme - #33153

Closed
arinjay wants to merge 5 commits into
react:mainfrom
arinjay:arinjay/iOSDebugTestFlightBugFix
Closed

fixed SDK issue while uploading app in debug scheme#33153
arinjay wants to merge 5 commits into
react:mainfrom
arinjay:arinjay/iOSDebugTestFlightBugFix

Conversation

@arinjay

@arinjayarinjay commented Feb 21, 2022

Copy link
Copy Markdown
Contributor

Summary

Problem - Error when trying to publish to Apple Store in debug scheme

Error thread - #31507

Changelog

[iOS][Changed] - The diffs renames the required variable which was causing conflicts in names with Apple core SDK's

@facebook-github-bot

Copy link
Copy Markdown
Contributor

Hi @arinjay!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@pull-bot

pull-bot commented Feb 21, 2022

Copy link
Copy Markdown
Messages
📖📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.

Generated by 🚫 dangerJS against 6cd06c6

@analysis-bot

analysis-bot commented Feb 21, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
ios-universaln/a--

Base commit: ed46ea2
Branch: main

@analysis-bot

analysis-bot commented Feb 21, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,188,328+415,607
androidhermesarmeabi-v7a7,788,450+610,516
androidhermesx868,558,277+476,782
androidhermesx86_648,511,318+449,549
androidjscarm64-v8a9,857,663+212,091
androidjscarmeabi-v7a8,842,331+422,770
androidjscx869,823,706+228,766
androidjscx86_6410,420,886+228,480

Base commit: ed46ea2
Branch: main

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 21, 2022
@facebook-github-bot

Copy link
Copy Markdown
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@facebook-github-botfacebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Feb 21, 2022
@facebook-github-bot

Copy link
Copy Markdown
Contributor

@SSHIC has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@arinjay

arinjay commented Feb 25, 2022

Copy link
Copy Markdown
ContributorAuthor

@lunaleaps can you help me in getting this MR merged

@kelset

Copy link
Copy Markdown
Contributor

@cortinico & @fortmarek this PR addressed the fact that if you try to upload to Apple's store a debug build, there seems to be an overlap of methods' names with the ones from the core iOS SDK. By changing them, it seems to address the problem - so I think overall it'd be a good change to integrate.
My only concern is that we might have to consider it a breaking change? I don't think we need to merge this for 0.69, but the issue has been open since May 2021 so it'd be good to get this sorted at some point.

(cc @tido64, do you have any opinions on this?)

@react-native-botreact-native-bot added the Platform: iOS iOS applications. label Apr 20, 2022
@fortmarek

Copy link
Copy Markdown
Contributor

My only concern is that we might have to consider it a breaking change?

The properties changed are with _ prefix and thus should be considered private. The only change which could be imho breaking is renaming of handleNotification to handlePackageNotification. Are we sure this is something we consider part of the public API? If so, it should be a breaking change, indeed.

I don't think there's a particular reason not to merge this change for 0.69.

Comment threadReact/Base/RCTKeyCommands.m Outdated
Comment threadReact/DevSupport/RCTPackagerClient.h Outdated
Comment threadReact/DevSupport/RCTPackagerConnection.mm Outdated
arinjayand others added 3 commits May 6, 2022 20:10
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
@cortinico

Copy link
Copy Markdown
Contributor

Looping @cipolleschi in here.

My only concern is that we might have to consider it a breaking change? I don't think we need to merge this for 0.69, but the issue has been open since May 2021 so it'd be good to get this sorted at some point.

IMHO we're fine merging this on main + mark it as a breaking change in the README.

I don't think there's a particular reason not to merge this change for 0.69.

Given the limited use case ("trying to publish to Apple Store in debug scheme") this is not pick-worthy for .69 imho.

@aakashchoubey

aakashchoubey commented May 7, 2022

Copy link
Copy Markdown

Given the limited use case ("trying to publish to Apple Store in debug scheme") this is not pick-worthy for .69 imho.

Just curious about the impact of this breaking change here. From what I understand, since iOS doesn't allow users to install apps as Android does with apks, Testflight is the only available option to upload an iOS build and test for non-ios developers.

Not supporting that is bad dev-exp right?

@cipolleschi

Copy link
Copy Markdown
Contributor

Thanks @cortinico. I agree, it is not pick worthy.

I'm also not so sure about it being breaking: is the RCTPackagerClient class supposed to be accessed and used by clients or should it be an internal details? In other terms, is it a private protocol or not? If the answer is yes, it definitely isn't a breaking change.

@choubeyaakash77, more or less yes, although I don't remember if we can actually submit something in Debug mode nor whether it is a good practice. By the way, the PR has been open since May 2021, as @kelset commented, so it is a year that this is broken. Waiting 1 more release won't hurt more than how it is already doing.

What do you think?

@aakashchoubey

Copy link
Copy Markdown

Waiting 1 more release won't hurt more than how it is already doing.

@cipolleschi Yeah it makes sense. If it is not a breaking change, there should be no problem in including it in 0.69, but if there is a breaking change, then that's a call on the react-native team I guess.

submit something in Debug mode nor whether it is a good practice

Submitting on TestFlight is different from submitting on AppStore. I am not sure but I don't think submitting something in Debug mode is a bad practice. That's a limitation given to us by Apple.
There is a valid use case imo to float around an app of a different variant than Release, to some internal users. There are many reasons for doing that - an app for backend developers, adding specific options for variants that won't be available in a release app, etc. One can potentially achieve the same by maintaining different branches/tags but that brings an overhead of maintaining multiple codebases. And this is fairly common in Android to make things available by app's variant.

@cipolleschi

Copy link
Copy Markdown
Contributor

Yeah, I see all the points and I agree with them.

What I was trying to say, failing, is that, if I remember well:

  • submitting an app to the App Store is one thing, and IIRC it must be built for release.
  • submitting an app for TestFlight should follow a different path and it can be sent in Debug mode.

So these warnings looks a little bit weird to me.

But again, it has a been a while since I had to submit something to Apple, and I was using an Enterprise profile to distribute the app internally in other context. So, I could remember wrong.

@cortinico

Copy link
Copy Markdown
Contributor

@arinjay Don't you mind updating the changelog entry to mention this is a breaking change?

@arinjay

Copy link
Copy Markdown
ContributorAuthor

@cortinico I can't see how it's a breaking change?

As per my understanding of the code, RCTPackagerClient is supposed to be an internal protocol used by SDK itself and not directly by the client.
So updating the method definition both at the protocol and implementation level can't break it on the client-side.

@arinjay

Copy link
Copy Markdown
ContributorAuthor

@cipolleschi yes you’re right

  1. submitting an app to the App Store (release mode) in one thing
  2. submitting an app for TestFlight(debug mode) is separate.

Schemes are different on iOS apps.

The need comes from the very limitation Apple has in the first place i.e. not being able to install IPA directly onto any iOS device.
This generally hampers the development cycle and the majority of folks have to resolve to release apps in debug mode on a test flight as not all end users have machines with Xcode.

Sharing it with multiple end-users and then gathering bugs becomes very easier. The whole iOS development cycle becomes more efficient.

@Tak-As

Copy link
Copy Markdown

When will this PR be merged?
I am facing this issue and am waiting for a merge.
I appreciate your help with moving this issue ahead.

@cortinico

Copy link
Copy Markdown
Contributor

When will this PR be merged?
I am facing this issue and am waiting for a merge.
I appreciate your help with moving this issue ahead.

Just to clarify, this won't be included in RN 0.69. The earliest this will be released in RN 0.70 as it's not considered a regression.

@react-native-bot

Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @arinjay in 086c13d.

When will my fix make it into a release? | Upcoming Releases

@react-native-botreact-native-bot added the Merged This PR has been merged. label Jun 6, 2022
@javache

Copy link
Copy Markdown
Contributor

I don't believe this is correct to fix and will revert this. This code is internal only and therefore can use private API's. You should never upload debug scheme builds to the App Store.

@javache

Copy link
Copy Markdown
Contributor

@cipolleschi and I look at an alternative way to address the issue in #31507. For now this breaks some development workflows, so we prefer to back this out.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request has been reverted by 6fcb878.

@arinjay

Copy link
Copy Markdown
ContributorAuthor

@javache what's the alternate way of resolving this? Can you mention that

Also "You should never upload debug scheme builds to the App Store"
This is correct
but we didn't upload it to AppStore, it failed to upload on the testFlight (these 2 are different mediums to publish app), even without the above MR changes.

Can you mention the changes using which we can still upload the app on testflight in debug mode?

@javache

Copy link
Copy Markdown
Contributor

You should never upload debug scheme builds to any distribution platform. They are inherently insecure as they allow any local packager instance to completely redefine your application's behaviour, while still providing access to privileged local storage (eg keychain).

@arinjay

Copy link
Copy Markdown
ContributorAuthor

'You should never upload debug scheme builds to any distribution platform'
Why is that?
Apple made testfight for this purpose only. People to whom the app is made available are aware of the access they will be sharing when downloading the app from testflight. Apple made sure to list out all potential risks before setting up test flight apps.

Use case in which debug mode is required - An app running both on react native and native (swift) codebase. now in debug mode we need to have one API call debugger library or profiler library which we only want in debug mode only, and installed dependencies in debug mode only. Post putting it on test flight to share it with other user, which in real-time can track the profiler data on actual devices or detect which API was fired.

These cases are very much required in many day-to-day app developments.

@gabriellend

gabriellend commented Aug 29, 2022

Copy link
Copy Markdown

@javache@arinjay Hi! Not sure this is still an ongoing conversation as the last reply was in June but I don't think it was implemented as I don't see it in the tags. I wanted to throw my two cents in as I can attest to the importance of being able to upload a debug scheme to TestFlight for internal testing before we submit a release version to the App Store. We've been doing it this way for years at my job (we have a staging environment and a production environment, so the debug scheme works off of staging and the release scheme works off production) and we now can't submit to TestFlight because of running into the selectors mentioned in this post. I've been in talks with Apple and they just got back to me directing me to manually change these myself so that the upload will be accepted to TestFlight. I want to voice my support for including this in the package. Thanks!

@rolfen

rolfen commented Mar 12, 2024

Copy link
Copy Markdown

@javache Whether you should upload debug scheme builds to the App Store or not is outside RN's scope.

This error message suggests an issue, either in the RN code or in Apple's validation.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.Platform: iOSiOS applications.RevertedShared with MetaApplied via automation to indicate that an Issue or Pull Request has been shared with the team.Type: Removal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

15 participants

@arinjay@facebook-github-bot@pull-bot@analysis-bot@kelset@fortmarek@cortinico@aakashchoubey@cipolleschi@Tak-As@react-native-bot@javache@gabriellend@rolfen@tido64
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fixed SDK issue while uploading app in debug scheme by arinjay · Pull Request #33153 · react/react-native · GitHub
Skip to content

fixed SDK issue while uploading app in debug scheme - #33153

Closed
arinjay wants to merge 5 commits into
react:mainfrom
arinjay:arinjay/iOSDebugTestFlightBugFix
Closed

fixed SDK issue while uploading app in debug scheme#33153
arinjay wants to merge 5 commits into
react:mainfrom
arinjay:arinjay/iOSDebugTestFlightBugFix

Conversation

@arinjay

@arinjayarinjay commented Feb 21, 2022

Copy link
Copy Markdown
Contributor

Summary

Problem - Error when trying to publish to Apple Store in debug scheme

Error thread - #31507

Changelog

[iOS][Changed] - The diffs renames the required variable which was causing conflicts in names with Apple core SDK's

@facebook-github-bot

Copy link
Copy Markdown
Contributor

Hi @arinjay!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@pull-bot

pull-bot commented Feb 21, 2022

Copy link
Copy Markdown
Messages
📖📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.

Generated by 🚫 dangerJS against 6cd06c6

@analysis-bot

analysis-bot commented Feb 21, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
ios-universaln/a--

Base commit: ed46ea2
Branch: main

@analysis-bot

analysis-bot commented Feb 21, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,188,328+415,607
androidhermesarmeabi-v7a7,788,450+610,516
androidhermesx868,558,277+476,782
androidhermesx86_648,511,318+449,549
androidjscarm64-v8a9,857,663+212,091
androidjscarmeabi-v7a8,842,331+422,770
androidjscx869,823,706+228,766
androidjscx86_6410,420,886+228,480

Base commit: ed46ea2
Branch: main

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 21, 2022
@facebook-github-bot

Copy link
Copy Markdown
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@facebook-github-botfacebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Feb 21, 2022
@facebook-github-bot

Copy link
Copy Markdown
Contributor

@SSHIC has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@arinjay

arinjay commented Feb 25, 2022

Copy link
Copy Markdown
ContributorAuthor

@lunaleaps can you help me in getting this MR merged

@kelset

Copy link
Copy Markdown
Contributor

@cortinico & @fortmarek this PR addressed the fact that if you try to upload to Apple's store a debug build, there seems to be an overlap of methods' names with the ones from the core iOS SDK. By changing them, it seems to address the problem - so I think overall it'd be a good change to integrate.
My only concern is that we might have to consider it a breaking change? I don't think we need to merge this for 0.69, but the issue has been open since May 2021 so it'd be good to get this sorted at some point.

(cc @tido64, do you have any opinions on this?)

@react-native-botreact-native-bot added the Platform: iOS iOS applications. label Apr 20, 2022
@fortmarek

Copy link
Copy Markdown
Contributor

My only concern is that we might have to consider it a breaking change?

The properties changed are with _ prefix and thus should be considered private. The only change which could be imho breaking is renaming of handleNotification to handlePackageNotification. Are we sure this is something we consider part of the public API? If so, it should be a breaking change, indeed.

I don't think there's a particular reason not to merge this change for 0.69.

Comment threadReact/Base/RCTKeyCommands.m Outdated
Comment threadReact/DevSupport/RCTPackagerClient.h Outdated
Comment threadReact/DevSupport/RCTPackagerConnection.mm Outdated
arinjayand others added 3 commits May 6, 2022 20:10
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
@cortinico

Copy link
Copy Markdown
Contributor

Looping @cipolleschi in here.

My only concern is that we might have to consider it a breaking change? I don't think we need to merge this for 0.69, but the issue has been open since May 2021 so it'd be good to get this sorted at some point.

IMHO we're fine merging this on main + mark it as a breaking change in the README.

I don't think there's a particular reason not to merge this change for 0.69.

Given the limited use case ("trying to publish to Apple Store in debug scheme") this is not pick-worthy for .69 imho.

@aakashchoubey

aakashchoubey commented May 7, 2022

Copy link
Copy Markdown

Given the limited use case ("trying to publish to Apple Store in debug scheme") this is not pick-worthy for .69 imho.

Just curious about the impact of this breaking change here. From what I understand, since iOS doesn't allow users to install apps as Android does with apks, Testflight is the only available option to upload an iOS build and test for non-ios developers.

Not supporting that is bad dev-exp right?

@cipolleschi

Copy link
Copy Markdown
Contributor

Thanks @cortinico. I agree, it is not pick worthy.

I'm also not so sure about it being breaking: is the RCTPackagerClient class supposed to be accessed and used by clients or should it be an internal details? In other terms, is it a private protocol or not? If the answer is yes, it definitely isn't a breaking change.

@choubeyaakash77, more or less yes, although I don't remember if we can actually submit something in Debug mode nor whether it is a good practice. By the way, the PR has been open since May 2021, as @kelset commented, so it is a year that this is broken. Waiting 1 more release won't hurt more than how it is already doing.

What do you think?

@aakashchoubey

Copy link
Copy Markdown

Waiting 1 more release won't hurt more than how it is already doing.

@cipolleschi Yeah it makes sense. If it is not a breaking change, there should be no problem in including it in 0.69, but if there is a breaking change, then that's a call on the react-native team I guess.

submit something in Debug mode nor whether it is a good practice

Submitting on TestFlight is different from submitting on AppStore. I am not sure but I don't think submitting something in Debug mode is a bad practice. That's a limitation given to us by Apple.
There is a valid use case imo to float around an app of a different variant than Release, to some internal users. There are many reasons for doing that - an app for backend developers, adding specific options for variants that won't be available in a release app, etc. One can potentially achieve the same by maintaining different branches/tags but that brings an overhead of maintaining multiple codebases. And this is fairly common in Android to make things available by app's variant.

@cipolleschi

Copy link
Copy Markdown
Contributor

Yeah, I see all the points and I agree with them.

What I was trying to say, failing, is that, if I remember well:

  • submitting an app to the App Store is one thing, and IIRC it must be built for release.
  • submitting an app for TestFlight should follow a different path and it can be sent in Debug mode.

So these warnings looks a little bit weird to me.

But again, it has a been a while since I had to submit something to Apple, and I was using an Enterprise profile to distribute the app internally in other context. So, I could remember wrong.

@cortinico

Copy link
Copy Markdown
Contributor

@arinjay Don't you mind updating the changelog entry to mention this is a breaking change?

@arinjay

Copy link
Copy Markdown
ContributorAuthor

@cortinico I can't see how it's a breaking change?

As per my understanding of the code, RCTPackagerClient is supposed to be an internal protocol used by SDK itself and not directly by the client.
So updating the method definition both at the protocol and implementation level can't break it on the client-side.

@arinjay

Copy link
Copy Markdown
ContributorAuthor

@cipolleschi yes you’re right

  1. submitting an app to the App Store (release mode) in one thing
  2. submitting an app for TestFlight(debug mode) is separate.

Schemes are different on iOS apps.

The need comes from the very limitation Apple has in the first place i.e. not being able to install IPA directly onto any iOS device.
This generally hampers the development cycle and the majority of folks have to resolve to release apps in debug mode on a test flight as not all end users have machines with Xcode.

Sharing it with multiple end-users and then gathering bugs becomes very easier. The whole iOS development cycle becomes more efficient.

@Tak-As

Copy link
Copy Markdown

When will this PR be merged?
I am facing this issue and am waiting for a merge.
I appreciate your help with moving this issue ahead.

@cortinico

Copy link
Copy Markdown
Contributor

When will this PR be merged?
I am facing this issue and am waiting for a merge.
I appreciate your help with moving this issue ahead.

Just to clarify, this won't be included in RN 0.69. The earliest this will be released in RN 0.70 as it's not considered a regression.

@react-native-bot

Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @arinjay in 086c13d.

When will my fix make it into a release? | Upcoming Releases

@react-native-botreact-native-bot added the Merged This PR has been merged. label Jun 6, 2022
@javache

Copy link
Copy Markdown
Contributor

I don't believe this is correct to fix and will revert this. This code is internal only and therefore can use private API's. You should never upload debug scheme builds to the App Store.

@javache

Copy link
Copy Markdown
Contributor

@cipolleschi and I look at an alternative way to address the issue in #31507. For now this breaks some development workflows, so we prefer to back this out.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request has been reverted by 6fcb878.

@arinjay

Copy link
Copy Markdown
ContributorAuthor

@javache what's the alternate way of resolving this? Can you mention that

Also "You should never upload debug scheme builds to the App Store"
This is correct
but we didn't upload it to AppStore, it failed to upload on the testFlight (these 2 are different mediums to publish app), even without the above MR changes.

Can you mention the changes using which we can still upload the app on testflight in debug mode?

@javache

Copy link
Copy Markdown
Contributor

You should never upload debug scheme builds to any distribution platform. They are inherently insecure as they allow any local packager instance to completely redefine your application's behaviour, while still providing access to privileged local storage (eg keychain).

@arinjay

Copy link
Copy Markdown
ContributorAuthor

'You should never upload debug scheme builds to any distribution platform'
Why is that?
Apple made testfight for this purpose only. People to whom the app is made available are aware of the access they will be sharing when downloading the app from testflight. Apple made sure to list out all potential risks before setting up test flight apps.

Use case in which debug mode is required - An app running both on react native and native (swift) codebase. now in debug mode we need to have one API call debugger library or profiler library which we only want in debug mode only, and installed dependencies in debug mode only. Post putting it on test flight to share it with other user, which in real-time can track the profiler data on actual devices or detect which API was fired.

These cases are very much required in many day-to-day app developments.

@gabriellend

gabriellend commented Aug 29, 2022

Copy link
Copy Markdown

@javache@arinjay Hi! Not sure this is still an ongoing conversation as the last reply was in June but I don't think it was implemented as I don't see it in the tags. I wanted to throw my two cents in as I can attest to the importance of being able to upload a debug scheme to TestFlight for internal testing before we submit a release version to the App Store. We've been doing it this way for years at my job (we have a staging environment and a production environment, so the debug scheme works off of staging and the release scheme works off production) and we now can't submit to TestFlight because of running into the selectors mentioned in this post. I've been in talks with Apple and they just got back to me directing me to manually change these myself so that the upload will be accepted to TestFlight. I want to voice my support for including this in the package. Thanks!

@rolfen

rolfen commented Mar 12, 2024

Copy link
Copy Markdown

@javache Whether you should upload debug scheme builds to the App Store or not is outside RN's scope.

This error message suggests an issue, either in the RN code or in Apple's validation.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.Platform: iOSiOS applications.RevertedShared with MetaApplied via automation to indicate that an Issue or Pull Request has been shared with the team.Type: Removal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

15 participants

@arinjay@facebook-github-bot@pull-bot@analysis-bot@kelset@fortmarek@cortinico@aakashchoubey@cipolleschi@Tak-As@react-native-bot@javache@gabriellend@rolfen@tido64
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); })(); fixed SDK issue while uploading app in debug scheme by arinjay · Pull Request #33153 · react/react-native · GitHub
Skip to content

fixed SDK issue while uploading app in debug scheme - #33153

Closed
arinjay wants to merge 5 commits into
react:mainfrom
arinjay:arinjay/iOSDebugTestFlightBugFix
Closed

fixed SDK issue while uploading app in debug scheme#33153
arinjay wants to merge 5 commits into
react:mainfrom
arinjay:arinjay/iOSDebugTestFlightBugFix

Conversation

@arinjay

@arinjayarinjay commented Feb 21, 2022

Copy link
Copy Markdown
Contributor

Summary

Problem - Error when trying to publish to Apple Store in debug scheme

Error thread - #31507

Changelog

[iOS][Changed] - The diffs renames the required variable which was causing conflicts in names with Apple core SDK's

@facebook-github-bot

Copy link
Copy Markdown
Contributor

Hi @arinjay!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@pull-bot

pull-bot commented Feb 21, 2022

Copy link
Copy Markdown
Messages
📖📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.

Generated by 🚫 dangerJS against 6cd06c6

@analysis-bot

analysis-bot commented Feb 21, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
ios-universaln/a--

Base commit: ed46ea2
Branch: main

@analysis-bot

analysis-bot commented Feb 21, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,188,328+415,607
androidhermesarmeabi-v7a7,788,450+610,516
androidhermesx868,558,277+476,782
androidhermesx86_648,511,318+449,549
androidjscarm64-v8a9,857,663+212,091
androidjscarmeabi-v7a8,842,331+422,770
androidjscx869,823,706+228,766
androidjscx86_6410,420,886+228,480

Base commit: ed46ea2
Branch: main

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 21, 2022
@facebook-github-bot

Copy link
Copy Markdown
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@facebook-github-botfacebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Feb 21, 2022
@facebook-github-bot

Copy link
Copy Markdown
Contributor

@SSHIC has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@arinjay

arinjay commented Feb 25, 2022

Copy link
Copy Markdown
ContributorAuthor

@lunaleaps can you help me in getting this MR merged

@kelset

Copy link
Copy Markdown
Contributor

@cortinico & @fortmarek this PR addressed the fact that if you try to upload to Apple's store a debug build, there seems to be an overlap of methods' names with the ones from the core iOS SDK. By changing them, it seems to address the problem - so I think overall it'd be a good change to integrate.
My only concern is that we might have to consider it a breaking change? I don't think we need to merge this for 0.69, but the issue has been open since May 2021 so it'd be good to get this sorted at some point.

(cc @tido64, do you have any opinions on this?)

@react-native-botreact-native-bot added the Platform: iOS iOS applications. label Apr 20, 2022
@fortmarek

Copy link
Copy Markdown
Contributor

My only concern is that we might have to consider it a breaking change?

The properties changed are with _ prefix and thus should be considered private. The only change which could be imho breaking is renaming of handleNotification to handlePackageNotification. Are we sure this is something we consider part of the public API? If so, it should be a breaking change, indeed.

I don't think there's a particular reason not to merge this change for 0.69.

Comment threadReact/Base/RCTKeyCommands.m Outdated
Comment threadReact/DevSupport/RCTPackagerClient.h Outdated
Comment threadReact/DevSupport/RCTPackagerConnection.mm Outdated
arinjayand others added 3 commits May 6, 2022 20:10
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
@cortinico

Copy link
Copy Markdown
Contributor

Looping @cipolleschi in here.

My only concern is that we might have to consider it a breaking change? I don't think we need to merge this for 0.69, but the issue has been open since May 2021 so it'd be good to get this sorted at some point.

IMHO we're fine merging this on main + mark it as a breaking change in the README.

I don't think there's a particular reason not to merge this change for 0.69.

Given the limited use case ("trying to publish to Apple Store in debug scheme") this is not pick-worthy for .69 imho.

@aakashchoubey

aakashchoubey commented May 7, 2022

Copy link
Copy Markdown

Given the limited use case ("trying to publish to Apple Store in debug scheme") this is not pick-worthy for .69 imho.

Just curious about the impact of this breaking change here. From what I understand, since iOS doesn't allow users to install apps as Android does with apks, Testflight is the only available option to upload an iOS build and test for non-ios developers.

Not supporting that is bad dev-exp right?

@cipolleschi

Copy link
Copy Markdown
Contributor

Thanks @cortinico. I agree, it is not pick worthy.

I'm also not so sure about it being breaking: is the RCTPackagerClient class supposed to be accessed and used by clients or should it be an internal details? In other terms, is it a private protocol or not? If the answer is yes, it definitely isn't a breaking change.

@choubeyaakash77, more or less yes, although I don't remember if we can actually submit something in Debug mode nor whether it is a good practice. By the way, the PR has been open since May 2021, as @kelset commented, so it is a year that this is broken. Waiting 1 more release won't hurt more than how it is already doing.

What do you think?

@aakashchoubey

Copy link
Copy Markdown

Waiting 1 more release won't hurt more than how it is already doing.

@cipolleschi Yeah it makes sense. If it is not a breaking change, there should be no problem in including it in 0.69, but if there is a breaking change, then that's a call on the react-native team I guess.

submit something in Debug mode nor whether it is a good practice

Submitting on TestFlight is different from submitting on AppStore. I am not sure but I don't think submitting something in Debug mode is a bad practice. That's a limitation given to us by Apple.
There is a valid use case imo to float around an app of a different variant than Release, to some internal users. There are many reasons for doing that - an app for backend developers, adding specific options for variants that won't be available in a release app, etc. One can potentially achieve the same by maintaining different branches/tags but that brings an overhead of maintaining multiple codebases. And this is fairly common in Android to make things available by app's variant.

@cipolleschi

Copy link
Copy Markdown
Contributor

Yeah, I see all the points and I agree with them.

What I was trying to say, failing, is that, if I remember well:

  • submitting an app to the App Store is one thing, and IIRC it must be built for release.
  • submitting an app for TestFlight should follow a different path and it can be sent in Debug mode.

So these warnings looks a little bit weird to me.

But again, it has a been a while since I had to submit something to Apple, and I was using an Enterprise profile to distribute the app internally in other context. So, I could remember wrong.

@cortinico

Copy link
Copy Markdown
Contributor

@arinjay Don't you mind updating the changelog entry to mention this is a breaking change?

@arinjay

Copy link
Copy Markdown
ContributorAuthor

@cortinico I can't see how it's a breaking change?

As per my understanding of the code, RCTPackagerClient is supposed to be an internal protocol used by SDK itself and not directly by the client.
So updating the method definition both at the protocol and implementation level can't break it on the client-side.

@arinjay

Copy link
Copy Markdown
ContributorAuthor

@cipolleschi yes you’re right

  1. submitting an app to the App Store (release mode) in one thing
  2. submitting an app for TestFlight(debug mode) is separate.

Schemes are different on iOS apps.

The need comes from the very limitation Apple has in the first place i.e. not being able to install IPA directly onto any iOS device.
This generally hampers the development cycle and the majority of folks have to resolve to release apps in debug mode on a test flight as not all end users have machines with Xcode.

Sharing it with multiple end-users and then gathering bugs becomes very easier. The whole iOS development cycle becomes more efficient.

@Tak-As

Copy link
Copy Markdown

When will this PR be merged?
I am facing this issue and am waiting for a merge.
I appreciate your help with moving this issue ahead.

@cortinico

Copy link
Copy Markdown
Contributor

When will this PR be merged?
I am facing this issue and am waiting for a merge.
I appreciate your help with moving this issue ahead.

Just to clarify, this won't be included in RN 0.69. The earliest this will be released in RN 0.70 as it's not considered a regression.

@react-native-bot

Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @arinjay in 086c13d.

When will my fix make it into a release? | Upcoming Releases

@react-native-botreact-native-bot added the Merged This PR has been merged. label Jun 6, 2022
@javache

Copy link
Copy Markdown
Contributor

I don't believe this is correct to fix and will revert this. This code is internal only and therefore can use private API's. You should never upload debug scheme builds to the App Store.

@javache

Copy link
Copy Markdown
Contributor

@cipolleschi and I look at an alternative way to address the issue in #31507. For now this breaks some development workflows, so we prefer to back this out.

@facebook-github-bot

Copy link
Copy Markdown
Contributor

This pull request has been reverted by 6fcb878.

@arinjay

Copy link
Copy Markdown
ContributorAuthor

@javache what's the alternate way of resolving this? Can you mention that

Also "You should never upload debug scheme builds to the App Store"
This is correct
but we didn't upload it to AppStore, it failed to upload on the testFlight (these 2 are different mediums to publish app), even without the above MR changes.

Can you mention the changes using which we can still upload the app on testflight in debug mode?

@javache

Copy link
Copy Markdown
Contributor

You should never upload debug scheme builds to any distribution platform. They are inherently insecure as they allow any local packager instance to completely redefine your application's behaviour, while still providing access to privileged local storage (eg keychain).

@arinjay

Copy link
Copy Markdown
ContributorAuthor

'You should never upload debug scheme builds to any distribution platform'
Why is that?
Apple made testfight for this purpose only. People to whom the app is made available are aware of the access they will be sharing when downloading the app from testflight. Apple made sure to list out all potential risks before setting up test flight apps.

Use case in which debug mode is required - An app running both on react native and native (swift) codebase. now in debug mode we need to have one API call debugger library or profiler library which we only want in debug mode only, and installed dependencies in debug mode only. Post putting it on test flight to share it with other user, which in real-time can track the profiler data on actual devices or detect which API was fired.

These cases are very much required in many day-to-day app developments.

@gabriellend

gabriellend commented Aug 29, 2022

Copy link
Copy Markdown

@javache@arinjay Hi! Not sure this is still an ongoing conversation as the last reply was in June but I don't think it was implemented as I don't see it in the tags. I wanted to throw my two cents in as I can attest to the importance of being able to upload a debug scheme to TestFlight for internal testing before we submit a release version to the App Store. We've been doing it this way for years at my job (we have a staging environment and a production environment, so the debug scheme works off of staging and the release scheme works off production) and we now can't submit to TestFlight because of running into the selectors mentioned in this post. I've been in talks with Apple and they just got back to me directing me to manually change these myself so that the upload will be accepted to TestFlight. I want to voice my support for including this in the package. Thanks!

@rolfen

rolfen commented Mar 12, 2024

Copy link
Copy Markdown

@javache Whether you should upload debug scheme builds to the App Store or not is outside RN's scope.

This error message suggests an issue, either in the RN code or in Apple's validation.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.Platform: iOSiOS applications.RevertedShared with MetaApplied via automation to indicate that an Issue or Pull Request has been shared with the team.Type: Removal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

15 participants

@arinjay@facebook-github-bot@pull-bot@analysis-bot@kelset@fortmarek@cortinico@aakashchoubey@cipolleschi@Tak-As@react-native-bot@javache@gabriellend@rolfen@tido64