Detect platform during init dataconnect:sdk instead of asking
#7522
Conversation
maneesht
left a comment
maneesht
left a comment
There was a problem hiding this comment.
Looks mostly good to me! I have a few discussion points, but I'll post them in chat rather than leave them here
yuchenshi
left a comment
yuchenshi
left a comment
There was a problem hiding this comment.
Edge case: If both web and Android indicators are present in the same directory, do we want to treat it as web (as implemented right now) or undetermined? If the former, what is the best order of priority?
dconeybe
left a comment
dconeybe
left a comment
There was a problem hiding this comment.
Two random comments:
- An irrelevant message about adding the project to Xcode is emitted when initializing the Kotlin SDK. Consider only emitting this message when configuring the Swift SDK:
i Generated SDK code for foo
i Please follow the instructions here to add your generated sdk to your XCode project:
https://firebase.google.com/docs/data-connect/gp/ios-sdk#set-client
- Any comments in the yaml file are discarded. I would expect comments to remain after "editing" the yaml file to add the new information.
src/init/features/dataconnect/sdk.ts
Outdated
| newConnectorYaml.generate.kotlinSdk?.outputDir || | ||
| path.relative( | ||
| connectorInfo.directory, | ||
| path.join(appDir, `generated/${newConnectorYaml.connectorId}`), |
There was a problem hiding this comment.
Omit ${newConnectorYaml.connectorId} from the output dir as the kotlin codegen will automatically append directories corresponding to the "package", which usually incorporates the connector name.
For example, I tested this out on a connector named "foo". The code was then generated into generated/foo/connectors/foo/ when it should have gone into generated/connectors/foo/.
src/init/features/dataconnect/sdk.ts
Outdated
| newConnectorYaml.generate.kotlinSdk?.outputDir || | ||
| path.relative( | ||
| connectorInfo.directory, | ||
| path.join(appDir, `generated/${newConnectorYaml.connectorId}`), |
There was a problem hiding this comment.
This is a good start, but for the vast majority of Android projects this output directory will be nonsensical. For small, single-module Android projects that use the conventional directory structure, a better output directory would be app/src/main. However, app is only a convention, and it can be named anything. Maybe consider looking for app/src/main and using that directory if it exists.
There was a problem hiding this comment.
Good idea - added a check for that directory. If its not found, should fall back to the appDir (the current behavior), or are there other structure we ought to try?
Good catch, will refactor to avoid that.
This one is an unfortunate side effect of us marshalling + unmarshalling the YAML to edit it. I think the effort required to avoid this is not worth the time ATM. Adding a TODO to remember this tho |
| connectorInfo.directory, | ||
| path.join(appDir, `generated/${newConnectorYaml.connectorId}`), | ||
| ); | ||
| path.relative(connectorInfo.directory, path.join(baseDir, `generated`)); |
There was a problem hiding this comment.
Do not add generated if app/src/main is used above. Only add generated if appDir is used above.
| // app/src/main is a common practice for Andorid, but not explicitly required. | ||
| // If it is present, we'll use it. Otherwise, we fall back to the app directory. | ||
| const baseDir = fs.existsSync(path.join(appDir, "app/src/main")) | ||
| ? path.join(appDir, "app/src/main") |
There was a problem hiding this comment.
I just realized, this should be app/src/main/java, not app/src/main.
| // If it is present, we'll use it. Otherwise, we fall back to the app directory. | ||
| const baseDir = fs.existsSync(path.join(appDir, "app/src/main")) | ||
| ? path.join(appDir, "app/src/main") | ||
| : appDir; |
There was a problem hiding this comment.
Suggest path.join(appDir, "generated") instead of appDir, then remove the logic to append "generated" from line 158 below.
|
My final comments were not addressed. Please fix as the output is suboptimal. Specifically, use |
4 participants
Description
Asking users to type in paths is the worst. Just knowing is better.
Still have a few more things to test here:
Scenarios Tested
Tested auto-detection of a web app, from the firebase.json directory and from the app directory
IOS (tested with
friendlyeats-iosapp)Android (tested with
friendlyeats-androidapp)