Skip to content

Minor tweaks to RCTDevSettings #696

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
nakambo merged 3 commits into from
Jan 26, 2021
Merged

Minor tweaks to RCTDevSettings #696

nakambo merged 3 commits into from
Jan 26, 2021

Conversation

@nakambo
Copy link
Collaborator

@nakambo nakambo commented Jan 26, 2021 •
edited by ghost
Loading

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Summary

  1. Export the not-debug\noop version of RCTDevSettings. Some Office code expects it to be present and barfs when it doesn't find it.
  2. Add a method to export all overridden dev setting keys. There's some (other) Office code that uses a custom data source, but without a way to get all the default keys (and their values) from the default RCTDevSettingsDataSource while replacing it, we end up unintentionally disabling certain things that would've been enabled were we to use the default data source.

Test Plan

  1. Verified that the noop version of RCTDevSettings has the desired effect.
Microsoft Reviewers: Open in CodeFlow

@nakambo nakambo requested a review from alloy as a code owner January 26, 2021 21:39
@pull-bot
Copy link

pull-bot commented Jan 26, 2021 •
edited
Loading

Messages
📖

📋 Missing Changelog - Can you add a Changelog? To do so, add a "## Changelog" section to your PR description. A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

DetailsCATEGORY may be:
  • General
  • macOS
  • iOS
  • Android
  • JavaScript
  • Internal (for changes that do not need to be called out in the release notes)

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against 0e6e125

@nakambo
Copy link
Collaborator Author

nakambo commented Jan 26, 2021

@HeyImChris is added to the review. #Closed

HeyImChris
HeyImChris suggested changes Jan 26, 2021
View reviewed changes
@ghost ghost removed the Needs: Author Feedback label Jan 26, 2021
@nakambo nakambo merged commit b3d41d3 into microsoft:master Jan 26, 2021
@nakambo nakambo deleted the nakambo/rctdevsettings branch January 26, 2021 22:29
HeyImChris pushed a commit that referenced this pull request Jan 27, 2021
@nakambo
Minor tweaks to RCTDevSettings (#696) (#697)
2bc9c5e 
* Export RCTDevSettings noop impl on shipping builds to JS

* Add a protocol method to get all set keys on dev settings

* tag changes
alloy
alloy reviewed Jan 27, 2021
View reviewed changes
React/CoreModules/RCTDevSettings.mm

@implementation RCTDevSettings

RCT_EXPORT_MODULE() // TODO(macOS ISS#2323203)
Copy link
Member

@alloy alloy Jan 27, 2021

Choose a reason for hiding this comment

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

This being missing seems a merge failure when I pulled in code from upstream, as it exists there. In that case the comment isn’t necessary.

Copy link
Collaborator Author

@nakambo nakambo Jan 27, 2021

Choose a reason for hiding this comment

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

ah, I see. I'll remove this if I touch this file in the near future.

HeyImChris pushed a commit to HeyImChris/react-native-macos that referenced this pull request Mar 3, 2021
@nakambo @HeyImChris
Minor tweaks to RCTDevSettings (microsoft#696)
0fe1230 
* Export RCTDevSettings noop impl on shipping builds to JS

* Add a protocol method to get all set keys on dev settings

* tag changes
HeyImChris added a commit that referenced this pull request Mar 3, 2021
@HeyImChris @nakambo
Minor devsettings fixes (#740)
2d363aa 
* Add nullability checks (#704)

* Update RCTCxxBridge.mm

* add nullability checks

* Minor tweaks to RCTDevSettings (#696)

* Export RCTDevSettings noop impl on shipping builds to JS

* Add a protocol method to get all set keys on dev settings

* tag changes

* podfile fix

Co-authored-by: Navneet Kambo <72474613+nakambo@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@alloy alloy alloy left review comments

@HeyImChris HeyImChris HeyImChris approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nakambo @pull-bot @alloy @HeyImChris