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

Fix/andriod 14 #5

Closed

Conversation

mzbyszynski
Copy link

DO NOT MERGE. WE KEEP THIS PR OPEN FOREVER.

Revisit this PR once we update to newer Notifee version.

Fixes:

  • Pulled in this approved notifee pr
  • Updated some android code to match what is currently in notifee to resolve some build runtime errors.

Checklist:

  • Build yarn then yarn build:all (Already done in this PR (200d697)
  • Use https://gitpkg.now.sh/rune/notifee/packages/react-native?COMMIT_HASH in package.json

Copy link

@OlDirtyLam OlDirtyLam left a comment

Choose a reason for hiding this comment

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

very nice 👏

@mieszko4
Copy link

mieszko4 commented Sep 10, 2024

  • Build yarn then yarn build:all (Already done in this PR (200d697)
  1. This points to another PR. I assume you meant to use 80b8d27?

  2. There is a mixture of commits with dist and non-dist changes so it is hard to see what actual changes came after applying feat: allow adding foreground service types dynamically invertase/notifee#1050

  3. It is not clear what version of Notifee we are patching

  4. Why is 6945232 needed?

@mzbyszynski
Copy link
Author

  • This points to another PR. I assume you meant to use 80b8d27?

Actually this was just a copy-paste mistake. I copied those steps from your last pr.

I agree! It sucks

  • It is not clear what version of Notifee we are patching

I agree! I think once Notifee merges that pr or otherwise fixes the android 14 issue we should try to move back to their official verison.

That was in the pr I merged in, but I couldn't see a reason for it and the app store wanted us to add a policy justification for it. I made a comment on the original pr about it

@mieszko4
Copy link

I agree! I think once Notifee merges that pr or otherwise fixes the android 14 issue we should try to move back to their official verison.

Ok, but we think the patch is over the newest Notifee, right? I am just concerned if all fixes in #3 are applied here. But maybe those fixes won't matter anymore in this PR.

@mzbyszynski
Copy link
Author

I agree! I think once Notifee merges that pr or otherwise fixes the android 14 issue we should try to move back to their official verison.

Ok, but we think the patch is over the newest Notifee, right? I am just concerned if all fixes in #3 are applied here. But maybe those fixes won't matter anymore in this PR.

I did branch this from that pr branch

@mzbyszynski
Copy link
Author

I think we were already using the commits from that pr in TinCan? Or was that a mistake

@mieszko4
Copy link

I think we were already using the commits from that pr in TinCan? Or was that a mistake

We are, but I think that the following is still not in the newest Notifee? At least it was not when I created #3. Not sure if it is still relevant though.

fix: always use startService: not in original lib yet (c05c7ca)

@mzbyszynski
Copy link
Author

I also just took another look at the notifee pr that I pulled the code from and that is not actually approved by any notifee maintainers, just some random person clicked approved I think 😞

@mieszko4
Copy link

I also just took another look at the notifee pr that I pulled the code from and that is not actually approved by any notifee maintainers, just some random person clicked approved I think 😞

It is merged and released in 9.0.0 :)

@mzbyszynski
Copy link
Author

mzbyszynski commented Oct 1, 2024

Can be closed now that TinCan is back on main notifee branch

@mzbyszynski mzbyszynski closed this Oct 1, 2024
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.

4 participants