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

[android] Remove a check that is no longer needed #535

Merged
merged 3 commits into from
Oct 16, 2024

Conversation

chrfalch
Copy link
Contributor

Summary

We've had some issue with monorepos when resolving the codegen config on Android in other libraries, and found that due to versions no longer in use we could now remove the version test in react-native-config.js and always return the component descriptor etc.

(The monorepo issue was with using require.main.require which will resolve to the calling script and not the module)

For reference, here is the same PR in @react-native-community/slider:

callstack/react-native-slider#657

Test Plan

This will not fix a bug as of now, but is a preparation for later to make sure that the component descriptors are correctly added to the autolinking.cpp file.

In @react-native-community/slider the following issue was registered: callstack/react-native-slider#652

We've had some issue with monorepos when resolving the codegen config on Android in other libraries, and found that due to versions no longer in use we could now remove the version test in `react-native-config.js` and always return the component descriptor etc.

(The monorepo issue was with using `require.main.require` which will resolve to the calling script and not the module)

For reference, here is the same PR in @react-native-community/slider:

callstack/react-native-slider#657
@jacobp100
Copy link
Collaborator

Thanks for this. I'm not too familiar with Android. Janic might know. Otherwise, we might just wait and see what CallStack do with the other PR?

@janicduplessis
Copy link
Member

I am a bit worried about backwards compat here, could we add a fallback to something like require("../@react-native-community/…") to handle that case? Assuming deps are in the same folder. I would also be open to just setting the default to true so if we cant find the file we assume codegenconfig is supported.

@chrfalch
Copy link
Contributor Author

I am a bit worried about backwards compat here, could we add a fallback to something like require("../@react-native-community/…") to handle that case? Assuming deps are in the same folder. I would also be open to just setting the default to true so if we cant find the file we assume codegenconfig is supported.

Understand your concerns - but would codegen even be used in an older version of React Native? We could also just change require.main.require -> require - which also works.

@chrfalch
Copy link
Contributor Author

The corresponding PR was merged in @react-native-community/slider now.

@janicduplessis
Copy link
Member

The problem that this code fixes is that older versions of RN cli do not support the codegen config and throw an error because of that extra config prop sadly.

@janicduplessis
Copy link
Member

If we want to keep this simple I would be fine just changing the default value so if the file is not found we add the codegen config.

Removed previous change and made the `supportsCodegenConfig` variable true initially
@chrfalch
Copy link
Contributor Author

If we want to keep this simple I would be fine just changing the default value so if the file is not found we add the codegen config.

Reverted and added a change to the initial value only as you suggested :)

@janicduplessis janicduplessis merged commit 8f99d4d into th3rdwave:main Oct 16, 2024
1 check passed
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