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

Update RNDateTimePickerShadowView.m #868

Merged
merged 5 commits into from
Mar 18, 2024

Conversation

bmichotte
Copy link
Contributor

Summary

This PR fixes #866 as suggested by @sbeigel in #866 (comment)

Test Plan

Build a RN app with xcode 15.3, you'll receive the error shown in #886

What's required for testing (prerequisites)?

What are the steps to reproduce (after prerequisites)?

Build in xcode 15.3

Compatibility

OS Implemented
iOS
Android

Checklist

  • I have tested this on a device and a simulator
  • I added the documentation in README.md
  • I updated the typed files (TS and Flow)
  • I added a sample use of the API in the example project (example/App.js)
  • I have added automated tests, either in JS or e2e tests, as applicable

@rahie-works
Copy link

@bmichotte I tried mocking this fix in my local to build an RN App in iOS 17.0.1. But I got these errors. Are these related?

Screenshot 2024-03-06 at 2 52 11 PM

@rahie-works
Copy link

@bmichotte I tried mocking this fix in my local to build an RN App in iOS 17.0.1. But I got these errors. Are these related?

Screenshot 2024-03-06 at 2 52 11 PM

Please disregard:
This was fixed from here itinance/react-native-sha256@77af268

@jzaefferer
Copy link

Thx! This patch worked for me.

We'll apply it locally for now with npx patch-package @react-native-community/datetimepicker

@f0ntana
Copy link

f0ntana commented Mar 8, 2024

Tks u!!
This worked for me too!
Please merge it.

Copy link

@ottaviano ottaviano left a comment

Choose a reason for hiding this comment

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

👍

Copy link

@OverGlass OverGlass left a comment

Choose a reason for hiding this comment

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

LGTM

@Robert27
Copy link

Is there any reason why this is not merged yet?

@fellipe-ribeiro
Copy link

Failed an automatic test.

I think it should be manually analyzed/approved and merged asap

@fnazarios
Copy link

Can we merge this asap?

@Robert27
Copy link

Is this repo still maintained?

@fortmarek fortmarek enabled auto-merge (squash) March 15, 2024 17:54
@fortmarek fortmarek disabled auto-merge March 15, 2024 17:55
@f0ntana
Copy link

f0ntana commented Mar 18, 2024

@luancurti @vonovak
Hi guys,
Could we have some attention here? tks.

@vonovak
Copy link
Member

vonovak commented Mar 18, 2024

Hello,
sorry for taking the time. The automated tests are failing, as you can see. We run automated tests in order to ensure the quality of what we're shipping.
What needs to happen is that someone needs to look into the problem; probably run the tests locally and probably update some dependencies like Detox / React Native / ... and maybe update the pipeline so that it runs on latest Xcode.

Do we have a volunteer here? 🙂 Alternatively, I'm happy to look into it but I'd really like to see some companies that use this module to step up and contribute some funds to the maintenance. Right now, the budget is $5 per month for a package that currently has 461 392 weekly downloads(!). I explained my stance in #313.
Thank you and sorry for being that guy.

@ottaviano
Copy link

Hi @vonovak,
Firstly, thanks a lot for your contribution to this library. I've noticed the CI for this PR is failing for the same reason the master branch's CI fails, indicating the issue isn't related to this PR specifically. Given the changes made here, particularly the fix to the type hint of the first argument, it seems important to merge this to address the significant bug affecting the library.

@vonovak
Copy link
Member

vonovak commented Mar 18, 2024

hi @ottaviano,
thanks for your comment. I didn't mean to imply that the CI failure is related to this change. If you look at the master branch history, you'll see it started failing for a reason that appears to be outside of this repository. In a pipeline that ran on the 15th of February, the e2e testing step passed okay (the publish step failed, but that's a different problem related to expired npm auth token, and I'm working on that). Something has changed in the mean time.

It doesn't happen often that stuff which worked previously would stop working, but it happened here. And, someone needs to do the work of fixing it (or disabling the pipeline checks, so that they are effectively ignored. That, however, would lead us down a slippery slope. Does anyone think disabling pipeline checks is a good idea?)

Even if I force-merge this PR, the change will not be released because the automated release depends on all pipeline steps completing successfully.

There is a known workaround using patch-package in apps consuming the package. The way I see it, people should use patch-package (or install from this branch) until we have all checks green. I'm sorry 🤷‍♂️ .

@bmichotte
Copy link
Contributor Author

Hello @vonovak, a quick search guided me to wix/Detox#4148 which is the same error.

The error on the android is related to a test killed after 1 hour since it took over than 40 minutes to start the emulator.

Try out a larger resource class or running tests in parallel to speed up job execution. Upgrade your pricing plan to take advantage of longer max job runtimes.
context deadline exceeded

As described in the Detox issue, updating Detox fix the issue, and therefore the iOS CI passes. I guess you can now merge this PR and release it :)

@vonovak vonovak merged commit 765ddf8 into react-native-datetimepicker:master Mar 18, 2024
4 of 5 checks passed
@vonovak
Copy link
Member

vonovak commented Mar 18, 2024

thank you for taking the initiative! 🙂

@ottaviano
Copy link

yeah! thanks to you two 🤝

vonovak pushed a commit that referenced this pull request Mar 18, 2024
## [7.6.3](v7.6.2...v7.6.3) (2024-03-18)

### Bug Fixes

* **ios:** use YGNodeConstRef in shadow view ([#868](#868)) ([765ddf8](765ddf8))
@vonovak
Copy link
Member

vonovak commented Mar 18, 2024

🎉 This issue has been resolved in version 7.6.3 🎉

If this package helps you, consider sponsoring us! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When running in Xcode 15.3 getting Incompatible function pointer types passing 'YGSize (...)' exception