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: ignore commented lines when finding libraryName #2542

Closed

Conversation

bang9
Copy link
Contributor

@bang9 bang9 commented Nov 4, 2024

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

  • Documentation is up to date to reflect these changes.
  • Follows commit message convention described in CONTRIBUTING.md

resolves #2545

@cortinico
Copy link
Member

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
Copy link
Contributor Author

bang9 commented Nov 4, 2024

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
Copy link
Contributor Author

bang9 commented Nov 4, 2024

@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
Copy link
Member

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
Copy link
Contributor Author

bang9 commented Nov 4, 2024

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
Copy link
Member

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
Copy link
Contributor Author

bang9 commented Nov 5, 2024

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!

@bang9 bang9 closed this Nov 5, 2024
@bang9 bang9 deleted the fix/findingLibraryName branch November 5, 2024 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants