Skip to content

Conversation

@tido64
Copy link
Member

@tido64 tido64 commented Sep 8, 2021

Description

When components is missing or empty in app.json, we attempt to populate the home screen with entries found in the AppRegistry.

Resolves #189.

Platforms affected

  • Android
  • iOS
  • macOS
  • Windows

Test plan

  1. Run yarn in example folder
    cd example
    yarn
    
  2. Remove components from app.json:
    diff --git a/example/app.json b/example/app.json
    index 8463354..0d50753 100644
    --- a/example/app.json
    +++ b/example/app.json
    @@ -1,17 +1,6 @@
     {
       "name": "Example",
       "displayName": "Example",
    -  "components": [
    -    {
    -      "appKey": "Example",
    -      "displayName": "App"
    -    },
    -    {
    -      "appKey": "Example",
    -      "displayName": "App (modal)",
    -      "presentationStyle": "modal"
    -    }
    -  ],
       "resources": {
         "android": [
           "dist/res",
  3. Build and run test app, e.g. for iOS:
    pod install --project-directory=ios
    yarn ios
    
    # In a separate terminal:
    yarn start
    

Remaining work

  • Since this solution relies on JSI, we cannot reuse it on Android without requiring the user to install NDK. React Native currently does not have this requirement, and we don't want to add extra dependencies. A solution would be to build and ship AARs like react-native does, but this will have to be a separate PR.

@github-actions github-actions bot added platform: iOS This affects iOS platform: macOS This affects macOS platform: Windows This affects Windows labels Sep 8, 2021
// loading. However, it seems that by posting a callback, we can
// defer fetching app keys until everything is done executing.
reactContext.JSDispatcher().Post([reactContext]() noexcept {
auto &runtime =
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if you get here when webdebugging?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a good call. The test app is broken due to an unrelated issue fixed in #493.

With the fix applied, what happens is that we return early with an empty array here:

auto global = runtime.global();
if (!global.hasProperty(runtime, kFbBatchedBridgeId)) {
return result;
}

I'm hesitant to implement a fallback for this, especially when web debugging might go away soon. Unless you have any suggestions, I'm going to leave this as is.

@tido64 tido64 mentioned this pull request Sep 16, 2021
31 tasks
@tido64 tido64 marked this pull request as ready for review September 16, 2021 10:18
@arazabishov
Copy link
Member

Could we send the list of components over the bridge instead? (to include android)

@tido64
Copy link
Member Author

tido64 commented Sep 17, 2021

Could we send the list of components over the bridge instead? (to include android)

We could, but that requires changes to the JS. I'm not sure how we can achieve that without requiring users to add an extra import or similar.

@tido64 tido64 added this to the 1.0 milestone Sep 22, 2021
}

let components = manifest.components
onComponentsRegistered(components, enable: false)
Copy link
Contributor

Choose a reason for hiding this comment

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

If there is a list of componetns in the manifest, wouldn't you want to call this with enable: true?

Copy link
Member Author

Choose a reason for hiding this comment

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

The property is for deciding whether the menu item should be enabled. Since the bridge hasn't been initialized yet, we should prevent them from being clicked. They get enabled when it is:

strongSelf.reactMenu.items.forEach { $0.isEnabled = true }

@tido64 tido64 merged commit 5f0f9fd into trunk Sep 27, 2021
@tido64 tido64 deleted the tido/auto-populate branch September 27, 2021 12:42
@github-actions
Copy link

🎉 This PR is included in version 0.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Labels

platform: iOS This affects iOS platform: macOS This affects macOS platform: Windows This affects Windows released

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Populate components list from AppRegistry if app.json is empty/missing

5 participants