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: add new arch #748

Merged
merged 15 commits into from
Mar 14, 2023
Merged

feat: add new arch #748

merged 15 commits into from
Mar 14, 2023

Conversation

WoLewicki
Copy link
Contributor

@WoLewicki WoLewicki commented Feb 15, 2023

Summary

PR adding new architecture support to the library. Last thing to be done is proper Permission type handling on iOS, so it is WIP.

Test Plan

What's required for testing (prerequisites)?

What are the steps to test it (after prerequisites)?

Compatibility

OS Implemented
iOS ✅❌
Android ✅❌

Checklist

  • I have tested this on a device and a simulator
  • I added the documentation in README.md
  • I added a sample use of the API in the example project (example/App.tsx)

@zoontek
Copy link
Owner

zoontek commented Feb 15, 2023

Hi @WoLewicki 👋 and thanks for the pull request.

Could you drop the FabricExample directory? It adds a lot of noise and I'd rather write a README.md inside the current example project explaining what's needed to be updated to enable the new arch 🙂

@WoLewicki
Copy link
Contributor Author

Ok, I'll do it when the code will work just fine on both archs 😅

@WoLewicki
Copy link
Contributor Author

@zoontek I believe the PR is ready for testing 🚀

@WoLewicki WoLewicki marked this pull request as ready for review February 24, 2023 09:56
@zoontek
Copy link
Owner

zoontek commented Feb 26, 2023

@WoLewicki Could you rebase against master? No worry, I took care of your podspec changes when shipping v3.7.0, you can resolve the conflict using you version.

(I also updated the example to react native 0.71, so it will be easier to review the PR)

@WoLewicki
Copy link
Contributor Author

@zoontek I did it. Looks like you can run new arch build on example project now 🚀

@zoontek
Copy link
Owner

zoontek commented Mar 14, 2023

@WoLewicki Everything looks good to me, thanks a lot!
I'm merging this, will plan a release during the week 🎉

"node_modules. You should add project extension property (in app/build.gradle) " +
"`REACT_NATIVE_NODE_MODULES_DIR` with path to react-native."
)
}

Choose a reason for hiding this comment

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

I am working with a monorepo using NPM workspaces and I can confirm that this implementation of resolveReactNativeDirectory() does not enable monorepo support.

Problem

The values for both reactNativeFromProjectNodeModules and reactNativeFromNodeModulesWithLibrary are the same!

In a monorepo setup there is likely to be a node_modules local to the package's folder, and a node_modules at the root of the monorepo. I suspect that the intention of having both a reactNativeFromProjectNodeModules and reactNativeFromNodeModulesWithLibrary was for one to point to each of the two possible locations where the react-native library might be located.

Setup

My workspaces is configured as such in the root package.json:

     "workspaces": [
       "apps/*"
     ]

I have a folder structure like this:

.
├── apps
│   └── rn-app
│       ├── android
│       ├── ios
│       ├── node_modules
│       │   └── react-native-permissions
│       ├── src
│       ├── index.js
│       ├── package.json
│       ├── react-native.config.js
│       └── tsconfig.json
├── node_modules
│   ├── react-native
│   │   └── ...
│   └── ...
├── README.md
├── package-lock.json
└── package.json

And, in the file, apps/rn-app/package.json I have a reference to react-native-permissions@3.8.4 under the dependencies.

Cause

Due to how NPM is resolving my dependencies my reference to react-native-permissions has not been hoisted to the node_modules at the root of my project (although react-native has been hoisted). Consequently, the exception message is triggering. Alas, I have no idea what the instruction of the exception message is asking me to do.

Choose a reason for hiding this comment

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

@trippingtarballs Hello!
I also have a monorepo and everything works for me. take a look at my implementation.
https://github.com/serserm/react-native-turbo-sensors

modified files:
react-native.config.js
metro.config.js
babel.config.js
settings.gradle
settings.gradle
tsconfig.json

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

Successfully merging this pull request may close these issues.

None yet

4 participants