react-native-community / cli

The React Native Community CLI - command line tools to help you build RN apps
MIT License
2.41k stars 905 forks source link

fix: ignore commented lines when finding libraryName #2542

Closed bang9 closed 2 weeks ago

bang9 commented 2 weeks ago

Summary:

This PR modifies the findLibraryName function to ignore lines that are commented out when searching for libraryName.

The library I maintain previously had libraryName commented out to prepare for support of the new architecture.

Now that the new architecture is enabled by default, during testing for the interop layer, this comment caused an issue where, despite being on the old architecture, the autolinking process treated it as a target for cmake. As a result, the Android build fails.

In conclusion, while removing the comment resolves the issue, it was quite challenging to trace back from the CMake and Gradle errors all the way to the CLI config extraction to identify the comment as the cause. This could potentially lead to issues for others, so I'm implementing this fix.

Test Plan:

  1. Verified that all existing tests pass successfully to ensure no regressions were introduced.
  2. Added new test cases specifically for handling commented lines within the findLibraryName function.
    • Checked that libraryName is found correctly, ignoring commented lines.
    • Verified that these new test cases pass as expected.

Checklist


resolves #2545

cortinico commented 2 weeks ago

This PR modifies the findLibraryName function to ignore lines that are commented out when searching for libraryName.

IMHO we should not merge this. If you have commented out lines with things like libraryName =, we should update the library code to remove those lines.

The CLI logic is already complicated trying to regex the libraries build logic, we should instead simplify libraries rather than making CLI attempt to parse every possible scenario

bang9 commented 2 weeks ago

Yes, I also disagree with having complex processing in the CLI.

However, from the perspective of managing a library, I don’t think it's necessary to know the detailed workings of auto-linking within react-native. In that case, at the very least, we should respect the developer's intentional code, as indicated by comments that specify it's not in use.

bang9 commented 2 weeks ago

@cortinico My library is scheduled for a patch, so I’m not strongly pushing for this change.

I believe the purpose of having an interop layer in the React Native ecosystem is to give community libraries sufficient time to prepare for the transition.

There may or may not be libraries like mine among them, but if there are, it’s the app developers—not the library maintainers—who will be affected. I wanted to prevent this from causing difficult-to-trace CMake build errors and significant time loss for the app developers using these libraries.

Please feel free to decide!

cortinico commented 2 weeks ago

In that case, at the very least, we should respect the developer's intentional code, as indicated by comments that specify it's not in use.

Developers can also use the react-native.config.js in their library to handle those scenarios

bang9 commented 2 weeks ago

In that case, at the very least, we should respect the developer's intentional code, as indicated by comments that specify it's not in use.

Developers can also use the react-native.config.js in their library to handle those scenarios

Of course, that’s true—if you know what the problem is.. In my case, however, I had to trace through several steps to understand the root cause: how auto-linking is handled, where it’s recognized as a new architecture module, which files are used to retrieve this list, and which scripts extract these files.. (the real issue is that the code with the developer's intent behaves implicitly in unintended ways due to the CLI.)

And those most affected by these issues and engaged in troubleshooting are likely to be users of libraries that are not actively maintained, rather than the maintainers themselves.

cortinico commented 2 weeks ago

Agree. Yet introducing the comment filtering for only one of the property parsing rather than all of them will be even more disrupting/confusing for users.

bang9 commented 2 weeks ago

Agree. Yet introducing the comment filtering for only one of the property parsing rather than all of them will be even more disrupting/confusing for users.

Oh... I just noticed that there are a lot of utility functions like findXXX in the config directory. I agree; it would be strange to apply changes only to this part.

Rather than making it more complex by applying the same patch to all functions, it might be more helpful to make the issue easier to find by documenting. I'll create an issue so it can be found through search engines, and then I'll close both the PR and the issue.

Thank you for the review!