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

Minor fixes #199

Merged
merged 31 commits into from
Dec 13, 2023
Merged

Minor fixes #199

merged 31 commits into from
Dec 13, 2023

Conversation

ospfranco
Copy link
Collaborator

@ospfranco ospfranco commented Nov 28, 2023

  • Improved README for installing and using babel-plugin-module-resolver
  • Improved the UI for the tests results, the text is no longer cut and the error can be clearly seen
  • Added a guard on MGLRandomHostObject when trying to cast a jsi arg to ArrayBuffer without making sure it is an array buffer
  • Removed simple tests (we already now the test framework works)
  • Fixed some minor typescript errors (maybe there to the new config from previous PR)
  • Added a guard on binaryLikeToArrayBuffer where the default encoding would try to create a Buffer with buffer encoding.
  • Changed naive check (input as any).buffer to ArrayBuffer.isView(input) on binaryLikeToArrayBuffer
  • Added a more robust casting on RandomFill to check for ArrayBufferViews/TypedArrays and getting the buffer out of them
  • Updated Github Actions to make the Android build pass
  • Removed the yarn.lock changes step on the actions since Github limited the permissions of the passed token

@ospfranco ospfranco self-assigned this Nov 28, 2023
@leahu-aurel
Copy link

@ospfranco, do you know when we can expect a new release with all the changes? It would be very helpful to not have to patch the package anymore and to have less warnings. Thanks for your work by the way!

@ospfranco
Copy link
Collaborator Author

Only @mrousavy has release permissions on the package I think

@leahu-aurel
Copy link

@mrousavy, thank you for contributing to this package! Do you have any ideas when all these changes can be released? I think that would be helpful for a lot of people!

@ospfranco
Copy link
Collaborator Author

please give me some time to polish this, no need to comment on every change

@ospfranco
Copy link
Collaborator Author

@mrousavy do you have time to review and release this?

@mrousavy
Copy link
Member

mrousavy commented Dec 4, 2023

Hey - yes I do, but I'm on vacation right now and it was just weekend.
I'll get back to y'all once I'm back from vacation

${{ runner.os }}-gradle-
- name: Run Gradle Build for android/
run: cd android && ./gradlew assembleDebug --build-cache && cd ..
# - name: Restore Gradle cache
Copy link
Member

Choose a reason for hiding this comment

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

Why it's commented out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

setup-java@v3 already has a cache logic integrated, see line 33

@@ -108,20 +99,11 @@ android {
}

buildTypes {
debug {
Copy link
Member

Choose a reason for hiding this comment

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

Why it was removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copied the latest build.gradle from builder.bob, I guess none of this options are no longer needed

Copy link
Member

Choose a reason for hiding this comment

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

Without this we won't be able to debug the library with android studio debugger

Copy link
Member

Choose a reason for hiding this comment

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

Let's add it back and I will merge the pr.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok

@@ -142,7 +124,22 @@ repositories {

dependencies {
//noinspection GradleDynamicVersion
implementation 'com.facebook.react:react-android:+'
implementation "com.facebook.react:react-native:+"
Copy link
Member

Choose a reason for hiding this comment

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

Will that break older rn versions?

Copy link
Member

Choose a reason for hiding this comment

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

Probably the opposite it will break versions above 0.71

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On older versions react-native:+ is kept. On versions above 0.71 is automatically replaced by react-android:+. This is done automatically by the react plugin

@Szymon20000
Copy link
Member

Thanks @ospfranco for the pr!
The pr looks good but I'm a bit worried about what are the consequences of updating build.gradle. Does it mean the library will only work on rn 0.72+?

@boorad boorad mentioned this pull request Dec 12, 2023
6 tasks
@ospfranco
Copy link
Collaborator Author

Debug config has been added back. Ready to merge @Szymon20000

@Szymon20000 Szymon20000 merged commit 0cea276 into margelo:main Dec 13, 2023
5 checks passed
@Szymon20000
Copy link
Member

Thanks @ospfranco!!!

@ospfranco ospfranco deleted the osp/minor-fixes branch December 13, 2023 12:18
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