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

Expo plugin #300

Merged
merged 68 commits into from
Jul 25, 2024
Merged

Expo plugin #300

merged 68 commits into from
Jul 25, 2024

Conversation

KennyHuRadar
Copy link
Contributor

@KennyHuRadar KennyHuRadar commented Mar 6, 2024

This PR introduces the following:

  1. expo plugins for react-native-radar
  2. example app that uses continuous native generation that tests correctness of expo plugin. Example app has been bumped to expo SDK 50.
  3. updated CI that checks correctness of plugins.

Note: The new example app opts to comment out the code for map libre's dependency and the map. This is due to the fact that there are some issues with the current map-libre's expo plugin. We are opting for our CI to test our code and expo plugin instead of map-libre's. That said, care should be taken when we are updating our RN map UI export and manual QA should be undertaken.

Docs: radarlabs/documentation#451

For this review, there seems to be a lot of lines of code changed but they are mostly auto-generated code from a new expo example app. The files that were modified within the example app are the package.json, app.json, App.tsx and exampleButton.js

@KennyHuRadar
Copy link
Contributor Author

@KennyHuRadar do you know if this will be merged anytime soon?

Yeah this PR did kind of get buried, @lmeier lets revisit this when you get back.

package.json Outdated Show resolved Hide resolved
@@ -16,6 +16,7 @@ jobs:
- run: npm run check-latest-tag
- run: npm run build
- run: npm run copy-assets
- run: npm run build-plugin
Copy link
Contributor

@ShiCheng-Lu ShiCheng-Lu Jul 2, 2024

Choose a reason for hiding this comment

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

we can probably just run build-all here + in pre-release action

@ShiCheng-Lu
Copy link
Contributor

If we are using expo managed build, we could consider removing android/ios in example app from git. Or we can keep it in as an example of what those files should look like for none-managed projects. (We can only track a subset of the files that are important, like the main .java/kt, Objective-C, manifests, and leave the image assets out)

@KennyHuRadar
Copy link
Contributor Author

If we are using expo managed build, we could consider removing android/ios in example app from git. Or we can keep it in as an example of what those files should look like for none-managed projects. (We can only track a subset of the files that are important, like the main .java/kt, Objective-C, manifests, and leave the image assets out)

A managed workflow will indeed relegate those native code to auto-generated code.

Flip side is that apps in the wild may be running without expo or use expo's bare workflow like we were previously doing. Removing our native folders may leave them out in the cold.

@ShiCheng-Lu
Copy link
Contributor

verified working with local install, we can make a pre-release just in case.

"web": "expo start --web",
"eject": "expo eject",
"postinstall": "patch-package"
"install-radar": "npm un --no-save react-native-radar ; npm i --no-save --install-links",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think --no-save works here without having react-native-radar in the dependencies, so just rm --no-save, and let dependencies be "react-native-radar": "file:.."

Copy link
Contributor

Choose a reason for hiding this comment

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

builds fine, but doesn't resolve RNRadar after the app opens, might just be me though

KennyHuRadar and others added 7 commits July 3, 2024 10:02
* we have maps again

* allow 10.0.0 alpha versions for peer dependency

* add maps back

* ios maps is also back

* un-move radar init from useEffect

* nvm it needs to be outside

* rm key

* Update RNRadar.m

---------

Co-authored-by: KennyHuRadar <139801512+KennyHuRadar@users.noreply.github.com>
Copy link
Contributor

@lmeier lmeier left a comment

Choose a reason for hiding this comment

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

A few questions:

  • should we add maplibre setup instructions (or a pointer to theirs?) to the docs PR? https://github.com/radarlabs/documentation/pull/451/files
  • what's the size diff on this PR? I'm also drawn to ShiCheng's suggestion that we potentially don't include the example apps native code in this—it feels like bloat. That shouldn't affect using the SDK at all.
  • some dependencies are now captured in this (e.g. the playstore integrity dep) — let's add some instructions to the react-native-sdk checklist about what potentially needs to be checked for that

target.build_phases.move(build_phase, 0)
end
# @generated begin @maplibre/maplibre-react-native-post_installer - expo prebuild (DO NOT MODIFY) sync-72b8976cc2231a4441a5b54389fb6e10bd42a1be
$RCTMLN.post_install(installer)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe callout that this is how you do it if you're on maplibre 10 or higher.

Is there any automatic podfile modifications we can do with the expo plugin? Or will we still need to mention to add this step?

Copy link
Contributor

Choose a reason for hiding this comment

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

the maplibre plugin manages this part, if you run expo prebuild, map libre will generate the correct post_install call into this Podfile.

Copy link
Contributor

Choose a reason for hiding this comment

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

we should add the maplibre plugin as well to the docs there (https://github.com/radarlabs/documentation/pull/451/files ) though (and make a comment that it's optional if they are not using maps)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool, made a note in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lmeier I think we can omit the native file, the only downside is that people people without expo will no longer be able to build our native folder and hence not be able to run our example app out of the box.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in terms of size diff, its honestly much smaller than it looks,
we are mostly

  • changing the CI
  • modifying the example app slightly to use expo and continued native genreation
  • implementing plugins.

most of the diff is just removing files.

Copy link
Contributor

Choose a reason for hiding this comment

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

image Looks like it's a 3mb difference, almost 20%.

Copy link
Contributor

Choose a reason for hiding this comment

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

note: it's actually 3.6mb, smaller than the previous 3.9mb. we're fine on this front

@KennyHuRadar KennyHuRadar merged commit 63b77aa into master Jul 25, 2024
3 checks passed
@KennyHuRadar KennyHuRadar deleted the expo-plugin branch July 25, 2024 19:41
@ChromeQ
Copy link

ChromeQ commented Jul 26, 2024

Will it be a requirement to add this plugin to the expo config?

"plugins": [
      [
        "react-native-radar",
        {
          "iosFraud": true,
          "iosNSLocationAlwaysAndWhenInUseUsageDescription": "test value",
          "iosBackgroundMode": true,
          "androidFraud": true,
          "androidBackgroundPermission": true,
          "androidFineLocationPermission": true
        }
      ],
      ["@maplibre/maplibre-react-native"]
    ]

Perhaps some documentation would be good for these options (and defaults) and whether maplibre is a requirement too?
Thanks

@KennyHuRadar
Copy link
Contributor Author

Will it be a requirement to add this plugin to the expo config?

"plugins": [
      [
        "react-native-radar",
        {
          "iosFraud": true,
          "iosNSLocationAlwaysAndWhenInUseUsageDescription": "test value",
          "iosBackgroundMode": true,
          "androidFraud": true,
          "androidBackgroundPermission": true,
          "androidFineLocationPermission": true
        }
      ],
      ["@maplibre/maplibre-react-native"]
    ]

Perhaps some documentation would be good for these options (and defaults) and whether maplibre is a requirement too? Thanks

No it will not be, it is only needed if you are also using Radar Maps. We have a documentation PR that will go live once we include this change in an upcoming release.

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.

5 participants