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

feat(macos): add macOS support for config plugins #2160

Merged
merged 4 commits into from
Aug 1, 2024

Conversation

tido64
Copy link
Member

@tido64 tido64 commented Jul 29, 2024

Description

I had this branch lying around for about a year or so. Rebased, and brushed up a bit.

Platforms affected

  • Android
  • iOS
  • macOS
  • visionOS
  • Windows

Test plan

  1. Enable config plugins:
    diff --git a/example/app.json b/example/app.json
    index 3b8992ea..8be080e1 100644
    --- a/example/app.json
    +++ b/example/app.json
    @@ -13,6 +13,9 @@
           "presentationStyle": "modal"
         }
       ],
    +  "plugins": [
    +    "./test-plugin.js"
    +  ],
       "resources": {
         "android": [
           "dist/res",
    diff --git a/example/package.json b/example/package.json
    index 35447f2e..b02cc291 100644
    --- a/example/package.json
    +++ b/example/package.json
    @@ -29,6 +29,7 @@
       "devDependencies": {
         "@babel/core": "^7.1.6",
         "@babel/preset-env": "^7.1.6",
    +    "@expo/config-plugins": "^8.0.0",
         "@react-native/babel-preset": "^0.73.19",
         "@react-native/metro-config": "^0.73.3",
         "@rnx-kit/metro-config": "^1.3.15",
  2. Create example/test-plugin.js:
    const { createRunOncePlugin, } = require("@expo/config-plugins");
    const { mergeContents } = require("@expo/config-plugins/build/utils/generateCode");
    const { macos } = require("react-native-test-app/plugins/index");
    
    function withCustomAppDelegate(config) {
      return macos.withAppDelegate(config, (config) => {
        config.modResults.contents = mergeContents({
          tag: "test-plugin.js",
          src: config.modResults.contents,
          newSrc: "// added by test-plugin.js",
          anchor: /import Cocoa/,
          offset: 1,
          comment: "//",
        }).contents;
        return config;
      });
    }
    
    module.exports = createRunOncePlugin(
      withCustomAppDelegate,
      "test-plugin.js",
      "UNVERSIONED"
    );
  3. Under example folder, run pod install --project-directory=macos
  4. Verify that AppDelegate.swift was written to:
    diff --git a/macos/ReactTestApp/AppDelegate.swift b/macos/ReactTestApp/AppDelegate.swift
    index 73a7288e..95f936ae 100644
    --- a/macos/ReactTestApp/AppDelegate.swift
    +++ b/macos/ReactTestApp/AppDelegate.swift
    @@ -1,4 +1,7 @@
     import Cocoa
    +// @generated begin test-plugin.js - expo prebuild (DO NOT MODIFY) sync-bf0d29fe87baaf78ed9ffca2f64bacb7c675534f
    +// added by test-plugin.js
    +// @generated end test-plugin.js
     import ReactTestApp_DevSupport
    
     @main

@github-actions github-actions bot added the platform: macOS This affects macOS label Jul 29, 2024
@tido64 tido64 force-pushed the tido/config-plugins/macos branch from 4887941 to 90ae790 Compare July 29, 2024 16:04
@tido64 tido64 changed the title fix(macos): add macos support for config plugins feat(macos): add macOS support for config plugins Jul 29, 2024
Comment on lines +19 to +20
// @ts-expect-error `macos` is not assignable to type `android | ios`
platform: "macos",
Copy link

@shirakaba shirakaba Jul 29, 2024

Choose a reason for hiding this comment

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

Unfortunately, Expo config plugins hard-code the string "ios" all over the place, consequentially searching in the ios folder and reading values from config.ios, so I'm not sure which plugins will work and which ones won't with this approach.

I opened a big PR last weekend to add macOS support to Expo config plugins, but I am not optimistic about it being reviewed anytime soon.

Copy link
Member Author

@tido64 tido64 Jul 30, 2024

Choose a reason for hiding this comment

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

If I understand this correctly, this part just registers what's available for the macos platform. We need to explicitly implement the mods separately, using withMod, as in plugins/macos.js.

Comment on lines +24 to +44
const defaultProviders = {
dangerous: expoProviders.dangerous,
finalized: expoProviders.finalized,
appDelegate: modifyFilePath(
expoProviders.appDelegate,
"ReactTestApp/AppDelegate.swift"
),
expoPlist: nullProvider,
xcodeproj: modifyFilePath(
expoProviders.xcodeproj,
"ReactTestApp.xcodeproj/project.pbxproj"
),
infoPlist: modifyFilePath(expoProviders.infoPlist, "Info.plist"),
entitlements: nullProvider,
podfile: makeNullProvider({
path: "",
language: /** @type {const} */ ("rb"),
contents: "",
}),
podfileProperties: makeNullProvider(),
};
Copy link

@shirakaba shirakaba Jul 30, 2024

Choose a reason for hiding this comment

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

This is really clever. 👏

Given RNTA has a predictable structure, a lot of the complexity in the plugins (e.g. searching for all possible locations for an Info.plist or Xcodeproj) can be totally sidestepped, as you've done here. And that side-steps a lot of the coupling to ios.

I am not sure to what extent this sidesteps all of their hard-coding of the ios platform, though. But given how your test passes, maybe it's quite a lot. Surely at least one of these will end up referring to config.ios or the ios folder at some point, though? Do you have an impression of the extent to which Expo config plugins will work?

Copy link
Member Author

@tido64 tido64 Jul 30, 2024

Choose a reason for hiding this comment

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

As far as I've understood, the providers are just functions for making modifications. They don't have props that are platform specific, but may be pointing to some iOS specific paths as you're suggesting. We should be overwriting all the props here (TypeScript would complain otherwise). Save for the null providers, I think we can say we are complete.

That said, these providers aren't supposed to be platform specific. The with* functions (e.g. withAppDelegate) are platform specific, and we need a separate set for macOS. That's what I've done here:

function withAppDelegate(config, action) {
return withMod(config, {
platform: macosPlatform,
mod: "appDelegate",
action,
});
}
/**
* Provides the `Info.plist` file for modification.
* @see {@link https://github.com/expo/expo/blob/sdk-51/packages/%40expo/config-plugins/src/plugins/ios-plugins.ts#L116}
* @param {ExportedConfig} config Exported config
* @param {Mod} action Method to run on the mod when the config is compiled
* @returns {ExportedConfig} Modified config
*/
function withInfoPlist(config, action) {
return withMod(config, {
platform: macosPlatform,
mod: "infoPlist",
async action(cfg) {
/** @type {ExportedConfigWithPropsMac} */
const config = await action(cfg);
if (!config.macos) {
config.macos = {};
}
config.macos.infoPlist = config.modResults;
return config;
},
});
}
/**
* Provides the main `.xcodeproj` for modification.
* @see {@link https://github.com/expo/expo/blob/sdk-51/packages/%40expo/config-plugins/src/plugins/ios-plugins.ts#L173}
* @param {ExportedConfig} config Exported config
* @param {Mod} action Method to run on the mod when the config is compiled
* @returns {ExportedConfig} Modified config
*/
function withXcodeProject(config, action) {
return withMod(config, {
platform: macosPlatform,
mod: "xcodeproj",
action,
});
}

Which means that for now, we only support these four functions. And you will need to import them from react-native-test-app/plugins/macos.js.

plugins/index.js Outdated Show resolved Hide resolved
@tido64 tido64 merged commit 4b9227d into trunk Aug 1, 2024
30 checks passed
@tido64 tido64 deleted the tido/config-plugins/macos branch August 1, 2024 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: macOS This affects macOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants