sparkfabrik / sparkfabrik-react-native-idfa-aaid

React Native module for getting IDFA (iOS) or AAID (Android)
MIT License
81 stars 22 forks source link

refactor (example app): migrate from vanilla RN app to RNTA #73

Closed kelset closed 8 months ago

kelset commented 8 months ago

๐Ÿ‘‹ Edo!

As promised, here's the PR that refactor the existing example app to use React Native Test App instead (which, if you look at the changes, reduces the complexity by a lot - the diff count is wonky because of the -lock file changes). A basic breakdown of the steps I took to do it can be found in this wiki page, and I've also updated the contributing docs to match.

On top of that, me and @tido64 did a couple of follow up tweaks to make the lib a bit cleaner and better:

Anyway, here's a screenshot of it all working locally:

Screenshot 2024-01-18 at 15 16 40

Try it out and let me know your thoughts!

edodusi commented 8 months ago

Hey @kelset! Thanks for the PR, I started testing it and found a potential issue with the Android run, not sure if it's related to my local setup or to something with RTNA but the way I fixed it makes me think it might be the latter.

So I ran npm run example:setup and it finished ok.

Then running npm run example:android led to errors because I forgot to run npm install on the root folder, so that's a step I would add on the documentation.

Oh, I also saw that after the setup an untracked file is created example/ios/.xcode.env.local that I think should be added to the .gitignore.

So after running npm install the Android setup starts, but it fails with this log:

FAILURE: Build failed with an exception.

* Where:
Script '/Users/edoardodusi/github/sparkfabrik-react-native-idfa-aaid/example/node_modules/react-native-test-app/test-app.gradle' line: 13

* What went wrong:
A problem occurred evaluating script.
> Failed to apply config plugins:
  node:internal/process/promises:289
              triggerUncaughtException(err, true /* fromPromise */);
              ^

  [Error: [ios.infoPlist]: withIosInfoPlistBaseMod: ENOENT: no such file or directory, open 'node_modules/.generated/ios/ReactTestApp/Info.plist'] {
    errno: -2,
    code: 'ENOENT',
    syscall: 'open',
    path: 'node_modules/.generated/ios/ReactTestApp/Info.plist'
  }

  Node.js v20.10.0

This seems strange because it mentions an iOS file Info.plist, but I'm running an Android build.

So I tried running npm run example:setup-ios, the setup finishes correctly, and then when I re-run npm run example:android the build finishes and I can run the app on the emulator.

And that's what made me think that it could be related to something in RNTA and not on my local setup. What do you think?

EDIT: after running the iOS setup another untracked file appears example/ios/Podfile.lock

edodusi commented 8 months ago

EDIT: Deleting this comment because I found that the issue was related to the fact that the new example app has a different name, so the workspace is different and pulling this resulted in having the old example workspace still there and empty, so nevermind

kelset commented 8 months ago

Ciao Edo, thanks for the report!

Then running npm run example:android led to errors because I forgot to run npm install on the root folder, so that's a step I would add on the documentation.

It's already there ;) https://github.com/kelset/sparkfabrik-react-native-idfa-aaid/blob/kelset/add-rnta-3/CONTRIBUTING.md#development-workflow

screenshot:

Screenshot 2024-01-19 at 13 35 21

re: for the local files yes, it looks like it's something local on your end, I can't see them. When changing branches I always recommend running git clean -fdx to ensure that you fully wipe the local files and only have what's tracked by git :)

re: the expo plugin issue, @tido64 is already on the case https://github.com/microsoft/react-native-test-app/pull/1784 :) thanks for noticing and the report! As soon as it's merged & released I'll bump the RNTA version in the deps and then we should be good to go :)

edodusi commented 8 months ago

Thanks @kelset and @tido64! I will re-run both apps when you update this, keep me posted ;)

kelset commented 8 months ago

@edodusi commit added, try again ;) (...but do a git clean -fdx first :D )

edodusi commented 8 months ago

@kelset tested with both builds and everything workked ๐ŸŽ‰

I will merge this. I was about to report the error with example:Android but I see @tido64 already opened the same issue on the CLI so I can only confirm.

Thanks!