FDC SDK init use the same outputDir pattern for both CLI and VS Code#7790
FDC SDK init use the same outputDir pattern for both CLI and VS Code#7790
Conversation
dataconnect-generated/{language}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7790 +/- ##
==========================================
+ Coverage 51.12% 51.14% +0.02%
==========================================
Files 418 419 +1
Lines 28908 28939 +31
Branches 5910 5904 -6
==========================================
+ Hits 14779 14801 +22
- Misses 12814 12823 +9
Partials 1315 1315
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
dataconnect-generated/{language}| const candidateDir = path.join(appDir, candidateSubdir); | ||
| if (fs.existsSync(candidateDir)) { | ||
| baseDir = candidateDir; | ||
| if (dirExistsSync(candidateDir)) { |
There was a problem hiding this comment.
Had to change this to use the helper because fs.existsSync don't work well work mock-fs
| connectorDir, | ||
| path.join(appDir, `dataconnect-generated/dart/${snakeCase(connectorYaml.connectorId)}`), | ||
| ), | ||
| package: snakeCase(connectorYaml.connectorId), |
There was a problem hiding this comment.
@maneesht brought this up in chat.
default isn't a valid dart package name, but out default connector name is default. Need a better pattern here.
There was a problem hiding this comment.
Updated the default package generation algorithm based on offline discussions.
ConnectorID: default
JS: @firebasegen/default-connector
Dart: default_connector
Android: connectors.default # this is fine as-is
IOS: DefaultConnector
|
Updated the default package. Ready for another pass |
| WEB = "WEB", | ||
| IOS = "IOS", | ||
| DART = "DART", | ||
| FLUTTER = "FLUTTER", |
| outputDir, | ||
| package: pkg, | ||
| packageJsonDir: newConnectorYaml.generate.javascriptSdk?.packageJsonDir, | ||
| outputDir: path.relative(connectorDir, path.join(appDir, `dataconnect-generated/js/${pkg}`)), |
There was a problem hiding this comment.
This seems different from the swiftSdk outputDir. Is there a reason for that?
There was a problem hiding this comment.
Aim for the same pattern dataconnect-generated/{js,swift,kotlin,dart}/${package} for all platforms.
${package} is part of outputDir of JS and Dart, but implicitly included for Swift and Kotlin.
I considered making JS and Dart automatically include package as well, but am afraid that would be a breaking change.
There was a problem hiding this comment.
Why would it be a breaking change? Isn't this just for when the user hasn't created a generated SDK for the platform yet?
|
Thanks for the detailed review! |
Description
Currently, VS Code button configure outputDir to
dataconnect-generatedunderappDir. CLI configures outputDir togenerated/${language}/${package}.I like the idea of the
dataconnect-generatedas the gen SDK convention. Clear name to remind customers that this is generated by dataconnect. We also should add a sub folder within it to signal language in case there are >1 apps in the folder.How about
dataconnect-generated/{js,swift,kotlin}/${package}as the gen SDK output path to consolidate both ideas?Note:
${package}is part ofoutputDirofJSandDart, but implicitly included forSwiftandKotlin.Updated the default package generation algorithm based on offline discussions.
default@firebasegen/default-connectordefault_connectorconnectors.defaultDefaultConnectorOther bug fixes:
Javascript.packageJsonDiris always set to appDir. There is little downside since emulator would ignore it gracefully. We should improve emulator logging though: http://cl/682785528Scenarios Tested
Added lots of unit tests for each platform. Run with
npx mocha src/dataconnect/fileUtils.spec.ts.Sample Commands