Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors configuration loading by dropping the deprecated and limited cosmiconfig in favor of a custom synchronous loader. Key changes include:
- Removal of cosmiconfig and lilconfig dependencies.
- Implementation of a custom loadConfig function for synchronous config loading.
- Updates to configuration loading in babel-config, init, and build scripts.
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/react-native-builder-bob/src/utils/loadConfig.ts | Implements a custom synchronous configuration loader, including special handling of package.json. |
| packages/react-native-builder-bob/src/init.ts | Updates config loading to use the new loadConfig with an explicit root parameter. |
| packages/react-native-builder-bob/src/build.ts | Uses the new loadConfig synchronously instead of cosmiconfig. |
| packages/react-native-builder-bob/babel-config.js | Replaces cosmiconfigSync with the new loadConfig and updates error handling accordingly. |
Files not reviewed (1)
- packages/react-native-builder-bob/package.json: Language not supported
Comments suppressed due to low confidence (2)
packages/react-native-builder-bob/src/utils/loadConfig.ts:65
- [nitpick] For errors other than MODULE_NOT_FOUND, the code rethrows the exception; consider adding an inline comment to clarify why non-module-not-found errors are rethrown to aid future maintainers.
throw e;
packages/react-native-builder-bob/babel-config.js:20
- The thrown error message ('Couldn't find a valid configuration.') might be too generic; consider specifying that no configuration file was found in the expected search places to improve clarity.
if (result == null) {
b515dca to
6c500e4
Compare
6c500e4 to
860bff6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cosmiconfig is no longer maintained. Apart from that, we can't support ESM configuration properly because
cosmiconfigonly supports loading it asynchronously - but we need to be able to load it synchronously in babel config.I also took a look at lilconfig, but it also has the same limitation.
Node 20.19.0 onwards supports synchronous
requireof ESM, so this limitation is not a technical limitation. So I decided to implement config loading myself since we don't need any advanced features of these config loaders such as traversing up until they find the config.Test plan
Tested in a sample project with both
bob buildand in the example app with metro.