Allow turbo-modules with names with @my-org/project-name #127
Conversation
There was a problem hiding this comment.
Thanks a ton for taking this on!
I tested this on https://github.com/unomed-dev/react-native-matrix-sdk using the example app and things work fine for iOS. On Android, the app appears to crash. At least I think it does. When I tap the button to make the native call in the app, it looks as if it goes into the background unexpectedly but then seems to relaunch entirely when I try to foreground it. I haven't seen this behaviour before and I'm not getting any related output on the terminal. 🤔
scripts/run-checkout-tests.sh
Outdated
| @@ -40,6 +40,7 @@ assert_eq $(git ls-remote --heads origin | grep refs/heads/releases/v1 | cut -f1 | |||
| popd | |||
|
|
|||
| clean_up | |||
| exit 0 | |||
There was a problem hiding this comment.
I think this might be a leftover?
There was a problem hiding this comment.
Yes! That was trying to get the error messages for #126 !
Removed now.
| --force-new-directory \ | ||
| --keep-directory-on-exit \ | ||
| --ubrn-config "$config" \ | ||
| --builder-bob-version 0.35.1 \ | ||
| --skip-ios \ | ||
| --skip-android \ | ||
| --slug @my-org/react-native-dummy-lib \ | ||
| "$working_dir/@my-org/react-native-dummy-lib" |
There was a problem hiding this comment.
The lines below use different indentation. Not that it matters at all of course. 😅
| --force-new-directory \ | |
| --keep-directory-on-exit \ | |
| --ubrn-config "$config" \ | |
| --builder-bob-version 0.35.1 \ | |
| --skip-ios \ | |
| --skip-android \ | |
| --slug @my-org/react-native-dummy-lib \ | |
| "$working_dir/@my-org/react-native-dummy-lib" | |
| --force-new-directory \ | |
| --keep-directory-on-exit \ | |
| --ubrn-config "$config" \ | |
| --builder-bob-version 0.35.1 \ | |
| --skip-ios \ | |
| --skip-android \ | |
| --slug @my-org/react-native-dummy-lib \ | |
| "$working_dir/@my-org/react-native-dummy-lib" |
crates/ubrn_cli/src/codegen/mod.rs
Outdated
| @@ -327,7 +327,7 @@ mod files { | |||
| templated_file!(PodspecTemplate, "module-template.podspec"); | |||
| impl RenderedFile for PodspecTemplate { | |||
| fn path(&self, project_root: &Utf8Path) -> Utf8PathBuf { | |||
| let name = self.config.project.raw_name(); | |||
| let name = self.config.project.cpp_filename(); | |||
There was a problem hiding this comment.
It strikes me as slightly odd that the filename for C++ is reused for the podspec. It doesn't really make a huge difference if the value is the same but maybe using a dedicated function would be cleaner?
|
I'll test out this build on my side @Johennes to see if I get similar results. I don't have a namespaced package like this, but I'm guessing the crash you are seeing is unrelated to that? |
Possibly though I'm not entirely sure since the organisation ends up in some generated names but not others. I'll try to also give it some more testing tomorrow. |
|
One thing I noticed here, it seems like it went from src/index.ts to src/index.tsx, so I had to update my package.json slightly. |
|
Yeah, I noticed that, too, but already had |
jhugman
force-pushed
the
jhugman/124-org-scoped-name-mangling
branch
from
788e563 to
99ee45d
Compare
jhugman
force-pushed
the
jhugman/124-org-scoped-name-mangling
branch
from
99ee45d to
96bb6ae
Compare
I found it! The Android crash was due to the library name in I think I broke it in #123, but might have broken it in this one. I've changed this and added a test. |
Our new tests check for differences between the generated files. Builder bob was generating an |
jhugman
force-pushed
the
jhugman/124-org-scoped-name-mangling
branch
from
96bb6ae to
579ed8a
Compare
|
Confirmed the fix is working! |
Fixes #124
According to The Big O of Code Reviews, this is a O(n) change.
This adds to the tests introduced in #123 to support names with an org name.