Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

WIP: Add react-native-system-setting #114

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

davidgovea
Copy link
Contributor

@davidgovea davidgovea commented Jun 3, 2019

This is a very handy plugin: react-native-system-setting.

The plugin config is pretty straightforward (no external deps), but it does rely on an advanced replaceInFile directive on iOS.

On iOS, bluetooth functionality is behind a preprocessor macro, so that it could be fully excluded and not expand the privacy requirements.

Our use-case required the bluetooth functionality, so we've hadrcoded the macro in the iOS configuration.

At first, I tried using a build-config container transform, but the macro needs to be set in the library's xcodeproj, not in ElectrodeContainer.

  // Does_not_work
  "containerGenerator": {
    "transformers": [
      {
        "name": "build-config",
        "extra": {
          "configurations": ["ElectrodeContainer-Release"],
          "settings": {
            "GCC_PREPROCESSOR_DEFINITIONS": "$(inherited) BLUETOOTH"
          }
        }
      },
      { "and another for": "Debug config" }

Since that didn't work, I fell back to a bare replace. 2 cases were needed to handle the syntax in debug & release settings.

Marked this as WIP -- definitely some things to discuss here.

  • Is there a better way to do a build-config edit on the imported plugin xcodeproj?
  • Is "bluetooth always on" an OK assumption to make? How would this be made dynamic?
  • What else needs to be defined in manifest.json? We've been using this config since ern@0.28 or so.

Thanks for taking the time to review this. Ern seems healthy -- we're excited to be using it!

@belemaire
Copy link
Member

belemaire commented Jun 3, 2019

Hi @davidgovea,

First of all, thanks for the support and contributions 👍

Regarding your first question, there is actually a way to add/update build settings of projects included in the Container, trough the pbxproj transformer.
This transformer was updated very recently with this new feature (as we needed such a feature as well internally), so you might have missed it ;)

For example, here is how we use it to set the BITCODE_GENERATION_MODE setting to bitcode in ALL projects (for Debug/Release configurations) included in the Container (that would have proven to be a big pain if we were to do that just using replace in file, as we have a lot of native modules in the container, so we had to come up with this solution) :

{
  "name": "ern-container-transformer-pbxproj",
  "extra": {
    "setBuildSettings": [
      {
        "targetProjectPath": "**/Libraries/**/*.xcodeproj",
        "buildSettings": [
          {
            "configurations": [
              "Debug",
              "Release"
            ],
            "settings": {
              "BITCODE_GENERATION_MODE": "bitcode"
            }
          }
        ]
      }
    ]
  }
}

You can update targetProjectPath to point to a specific project only (in this case the RCTSystemSetting.xcodeproj).

I assume this should also answer your second bullet point.
We shouldn't keep the replaceInFile in the public manifest plugin configuration. I can see that this plugin is not only about bluetooth but also other 'settings' that for some require permission. We cannot just enable all of them in the plugin config, this is not clean, so they should be enabled dynamically as you say. For now one way (at least for iOS) is through pbxproj transformer. Another way is to keep the replaceInFile statements. Not in the public manifest, but in your own override Manifest.

For Android, we would probably have to add some kind of new transformer (something like an AndroidManifest transformer) to update the container manifest to inject permissions dynamically (for now though it could be achieved by using the permissions directive of android plugin configuration to inject permissions, but in that case only possible through override plugin configuration in an override manifest.

That being said, even though you could use the transformer for iOS, rather than replaceInFile, I would still use the replaceInFile in an override manifest. The reason being that if you keep everything related to the plugin in its plugin configuration file, it's the single source of truth and makes things clear. If for a plugin you have a config + a transformer, it's harder to manage. For example If you get rid of the plugin you also need to keep in mind that you have to get rid of the transformer. And the transformer 'config' is split in two different places, which is not ideal. Also users not using a Cauldron to generate a Container (just run-ios for example with standalone MiniApp) would face a problem as no transformer will be run in that case.

But the replaceInFile is not ideal though, it's a bit verbose and sometimes hard to get right. We should probably introduce a new plugin configuration directive buildSettings for example, to allow adding/updating build settings of the plugin in the same way we do it with the transformer. I'll add a note to look into that.

So, to put it in a nutshell :

  • Remove replaceInFile in the plugin configuration
  • Either use the pbxProj transformer to update needed build settings of the plugin project, or an override manifest for the plugin config, using replaceInFile as you do now. That are the proper ways to dynamically update such project build settings for now.
  • We will add a buildSettings directive for iOS plugin configuration, to make things more straightforward than having to rely on replaceInFile
  • We will also think of better ways to handle such dynamic settings without having to rely on a transformer or override plugin configuration in custom manifest.

As for the last bullet point, you should just add react-native-system-setting with its latest version at the time of addition, to the latest platform version currently available in manifest (0.32.x) as well as the development version (1000.0.x). As a rule of thumb, we only support current latest released version (through patch releases), so similarly we only add new plugins support to current version. That also gives an incentive to update to latest ;) (unless you use your own override manifest).

@davidgovea
Copy link
Contributor Author

@belemaire thank you so much for the detailed response! Makes complete sense.

Great tips on transformer-vs-plugin, too: I'll play around with the newer pbxProj features.

Next steps for me, for this PR:

  • Remove replaceInFile
  • Add README to plugin folder, detailing steps for adding preprocessor statements via custom manifest or transform
  • Add manifest entry to latest stable ern version

@belemaire
Copy link
Member

@davidgovea
Yes you are right we can also add a README at the root of the plugin configuration when there are certain important details to be aware of for this plugin. We started doing that with realm plugin config. Thanks for taking care of that !
FYI we are planning on releasing 0.33 later on today, and I should prob be able to squeeze in the new buildSettings directive for iOS plugin configuration, so hold off this PR/README for tomorrow I would recommend, so that we can just refer to this new directive rather than replaceInFile.

@belemaire
Copy link
Member

@davidgovea
FYI we just release Electrode Native 0.33 which adds a new directive setBuildSettings for iOS plugin configuration.
This makes things a little bit less verbose, and easier to get right than using replaceInFile as can be seen below:

setBuildSettings": [
  { 
    "path": "{{{projectName}}}/Libraries/RCTSystemSetting/RCTSystemSetting.xcodeproj/project.pbxproj",
    "buildSettings": {
      "configurations": ["Debug", "Release"],
      "settings": { "GCC_PREPROCESSOR_DEFINITIONS": "BLUETOOTH" }
    }
  }
]

You can add this to the README.md file that you'll add, rather than replaceInFile.
Because this new directive is only part of Electrode Native 0.33 and above, I created a new directory ern_v0.33.0+ in the Manifest, please move this plugin inside this directory rather than ern_v0.14.0+.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants