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: RN 73 compatibility #726

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Conversation

felipecsl
Copy link

@henninghall
Copy link
Owner

henninghall commented Dec 3, 2023

@felipecsl thanks a lot for the PR! 💛

  1. I don't see the gradle.properties change being mentioned in the referenced discussion, is it really needed?
  2. The unit tests are failing after this change, would you mind taking a look at it?

@felipecsl
Copy link
Author

@henninghall the gradle.properties change is needed because on AGP8, BuildConfig is no longer enabled by default, which breaks the build. Adding this flag reenables it.

The unit tests are failing exactly for this reason, you need to add the same flag here https://github.com/henninghall/react-native-date-picker/blob/master/examples/Rn072/android/gradle.properties

Fix package
@felipecsl
Copy link
Author

There was another issue related to the namespace not matching the R class package. Just updated which should fix it

@felipecsl
Copy link
Author

Updated both. Please keep in mind that this is a breaking change due to the AndroidManifest.xml change, so you'll have to bump the library major version when this is released in order to adhere to semver rules

@henninghall
Copy link
Owner

@felipecsl I tried the newly released version of RN 0.73 and the package seems to run fine without this fix. Do you know how to reproduce the issue?

Added an Rn073 example project in the repo.

@felipecsl
Copy link
Author

Interesting. this did pop up with a project I was trying to upgrade, but we put the upgrade on hold for now so I'm no longer actively working on it.
I'd say let's wait to see if more users will run into this issue, no rush to merge yet I think.

@henninghall
Copy link
Owner

henninghall commented Dec 11, 2023

Apparently this change doesn’t seem to be necessary, the reason is that the react native team added a backward compatibility for this, which is great.

They still recommend doing this change, but I would like to postpone is as long as possible since it introduces a breaking change and additional work of having to maintain 2 different branches of the package.

I put this PR as as draft meanwhile.

@henninghall henninghall marked this pull request as draft December 11, 2023 13:32
@rvera
Copy link

rvera commented Feb 6, 2024

Are your fixes on the latest version? Somehow I'm still getting a Namespace not specified error.

(Trying to update to RN 73, using v4.3.5 of this lib)

@felipecsl
Copy link
Author

These changes have been not merged yet, so I assume no

@henninghall
Copy link
Owner

@rvera could you please share an example repo where the issue is present? As far as I know RN 0.73 is supported without this PR and an example repo with RN 0.73 with the the picker is provided in the example folder of this repo.

@VelocityPulse
Copy link

VelocityPulse commented Apr 11, 2024

Is this pull request abandonned ?
We actually need it for React native 73

@henninghall
Copy link
Owner

@VelocityPulse please read comment #726 (comment)

Can you provide a repo where this is actually an issue? Then we can look into it again.

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