Uh oh!
There was an error while loading. Please reload this page.
feat: use config inside run-android - #791
Conversation
9a64c8a to
9479050Compare9479050 to
6787e3eCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
grabbou
left a comment
There was a problem hiding this comment.
LGTM. Left some commends and ideas, open to discussions :) Good job!
Uh oh!
There was an error while loading. Please reload this page.
| @@ -359,6 +366,7 @@ function startServerInNewWindow( | |||
| logger.error( | |||
| `Cannot start the packager. Unknown platform ${process.platform}`, | |||
There was a problem hiding this comment.
Shouldn't we throw here?
TIP: This codeebase hasn't been touched in a while, I am pretty sure it has some wild logger.error instead of CLIError in few places.
Good opportunity to update.
There was a problem hiding this comment.
It's not related to the PR, we can take it separately under "improving error messags and handling inside run-android". Whatever makes more sense!
| const packageMatch = manifestContent.match(/package="(.+?)"/); | ||
| if (packageMatch) { | ||
| packageName = packageMatch[1]; |
There was a problem hiding this comment.
One more thing, don't we already have a packageName in the configuration?
I would also consider returning manifestPath from the configuration as well, as we already use it to derive packageName.
Otherwise, we risk using two different AndroidManifests which might be dangerous.
There was a problem hiding this comment.
I see we load AndroidManifest here based on args.appFolder, which by default is "app" (that's what configuration uses here https://github.com/react-native-community/cli/blob/next/packages/platform-android/src/config/findAndroidAppFolder.ts).
My proposal:
- make
AndroidProjectParamsacceptappFolderwith default value beingapp - here, in this code, check if
args.appFolderis provided and then, callprojectConfigwith{...androidProjectConfig, appFolder: args.appFolder}to get a "refreshed" configuration.
Would keep this unified I think. What you think?
There was a problem hiding this comment.
Yes we have, but the tricky thing is the appFolder. Gradle has a concept of root project (where root build.gradle is) and sub projects (e.g. app, core, ui). In a typical RN app, in settings.gradle we only include app project. But technically, in an Android-first project, there could be many gradle sub projects – and hence appFolder. Our configuration doesn't have this as an option. We could add it, but even when overwritten through a flag, the npx react-native config won't know about it. This is likely a super edge-case or even not happening, but we still support it.
That's why I can't really use the packageName or even appId from the configuration – because they only refer to a single project.
489cae3 to
455dcbdComparegrabbou
commented
Nov 15, 2019
Any update on this PR? The tests are failing, not reviewing at this point. |
thymikee
commented
Nov 15, 2019
Didn't have time to revisit yet |
455dcbd to
1d74191Compared5a5757 to
0901b2eComparethymikee
commented
Mar 3, 2020
Superseded by #1029 |
Summary:
Defer reading as much as possible from the shared config.
Details:
rootoption in favor of deriving it from config.sourceDirto only look forandroid/directory, which is a Gradle root project – this alignssourceDirto iOS and allows for greater flexibility, becauseandroid/appwas a hardcoded name for a defaultappFolder, which would likely break stuff when somebody started usingappFolderflag--appFolderand--appIdflagsandroid.project.appNameto compensate forappFolderappIdSuffix, when app uses flavorsFixes#781.
Test Plan:
Updated tests.