Make FDC SDK init default outputDir disjoint #7546

fredzqm · 2024-08-09T00:26:16Z

Description

The existing behavior

From an empty directory. Run firebase init dataconnect and then firebase init dataconnect:sdk 3 times for each platform.

We got connector.yaml:

connectorId: default-connector
authMode: PUBLIC
generate:
  swiftSdk:
    outputDir: ../../generated/default-connector
    package: defaultConnector
  javascriptSdk:
    outputDir: ../../generated/default-connector
    package: "@firebasegen/default-connector"
  kotlinSdk:
    outputDir: ../../generated
    package: connectors.default_connector

Then firebase dataconnect:sdk:generate fails with

I0808 17:27:31.361713   50729 codegen.go:78] [connector "default-connector" javascriptSdk] Generating sources into "/Users/fredzqm/dev/fdc-free/generated/default-connector"
I0808 17:27:31.369158   50729 codegen.go:78] [connector "default-connector" kotlinSdk] Generating sources into "/Users/fredzqm/dev/fdc-free/generated/connectors/default_connector"
I0808 17:27:31.371549   50729 codegen.go:78] [connector "default-connector" swiftSdk] Generating sources into "/Users/fredzqm/dev/fdc-free/generated/default-connector/defaultConnector"
E0808 17:27:31.373203   50729 cmd.go:45] Failed to generate SDKs:
outputDir must be disjoint, but found "/Users/fredzqm/dev/fdc-free/generated/default-connector" [connector "default-connector" javascriptSdk] is a parent folder of "/Users/fredzqm/dev/fdc-free/generated/default-connector/defaultConnector" [connector "default-connector" swiftSdk]

Using Google Analytics in DEBUG mode. Emulators (+ UI) events will be shown in GA Debug View only.

Error: Unable to generate your Data Connect SDKs (exit code 1): I0808 17:27:31.361713   50729 codegen.go:78] [connector "default-connector" javascriptSdk] Generating sources into "/Users/fredzqm/dev/fdc-free/generated/default-connector"
I0808 17:27:31.369158   50729 codegen.go:78] [connector "default-connector" kotlinSdk] Generating sources into "/Users/fredzqm/dev/fdc-free/generated/connectors/default_connector"
I0808 17:27:31.371549   50729 codegen.go:78] [connector "default-connector" swiftSdk] Generating sources into "/Users/fredzqm/dev/fdc-free/generated/default-connector/defaultConnector"
E0808 17:27:31.373203   50729 cmd.go:45] Failed to generate SDKs:
outputDir must be disjoint, but found "/Users/fredzqm/dev/fdc-free/generated/default-connector" [connector "default-connector" javascriptSdk] is a parent folder of "/Users/fredzqm/dev/fdc-free/generated/default-connector/defaultConnector" [connector "default-connector" swiftSdk]

Scenarios Tested

> cat dataconnect/default-connector/connector.yaml 
connectorId: default-connector
authMode: PUBLIC
generate:
  javascriptSdk:
    outputDir: ../../generated/javascript/default-connector
    package: "@firebasegen/default-connector"
    packageJsonDir: ../..
  swiftSdk:
    outputDir: ../../generated/swift
    package: DefaultConnector
  kotlinSdk:
    outputDir: ../../generated/kotlin
    package: connectors.default_connector

> tree generated
generated
├── javascript
│   └── default-connector
│       ├── esm
│       │   ├── index.esm.js
│       │   └── package.json
│       ├── index.cjs.js
│       ├── index.d.ts
│       └── package.json
├── kotlin
│   └── connectors
│       └── default_connector
│           ├── DefaultConnectorConnector.kt
│           └── DefaultConnectorConnectorKeys.kt
└── swift
    └── DefaultConnector
        ├── DefaultConnectorClient.swift
        ├── DefaultConnectorKeys.swift
        ├── DefaultConnectorOperations.swift
        └── Package.swift

9 directories, 11 files

When app/src/main/kotlin is present:

> cat dataconnect/default-connector/connector.yaml 
connectorId: default-connector
authMode: PUBLIC
generate:
  javascriptSdk:
    outputDir: ../../generated/javascript/default-connector
    package: "@firebasegen/default-connector"
    packageJsonDir: ../..
  swiftSdk:
    outputDir: ../../generated/swift
    package: DefaultConnector
  kotlinSdk:
    outputDir: ../../app/src/main/kotlin
    package: connectors.default_connector

> tree .
.
├── app
│   └── src
│       └── main
│           └── kotlin
│               └── connectors
│                   └── default_connector
│                       ├── DefaultConnectorConnector.kt
│                       └── DefaultConnectorConnectorKeys.kt
├── build.gradle
├── dataconnect
│   ├── dataconnect.yaml
│   ├── default-connector
│   │   ├── connector.yaml
│   │   ├── mutations.gql
│   │   └── queries.gql
│   └── schema
│       └── schema.gql
├── firebase.json
├── generated
│   ├── javascript
│   │   └── default-connector
│   │       ├── esm
│   │       │   ├── index.esm.js
│   │       │   └── package.json
│   │       ├── index.cjs.js
│   │       ├── index.d.ts
│   │       └── package.json
│   └── swift
│       └── DefaultConnector
│           ├── DefaultConnectorClient.swift
│           ├── DefaultConnectorKeys.swift
│           ├── DefaultConnectorOperations.swift
│           └── Package.swift
├── package.json
└── podfile

16 directories, 20 files

fredzqm · 2024-08-09T00:32:22Z

src/init/features/dataconnect/sdk.ts

-      if (fs.existsSync(candidateDir)) {
-        baseDir = candidateDir;
-      }
-    }


This isn't needed. @dconeybe and I made it always append the package path after the output path.

Just did the same for Swift as well.

Did you chat about this recently? @dconeybe requested this behavior in comments on #7522

Yeah, this was fairly recent.

He actually added this logic to emulator.

@fredzqm, this logic was added by me in #7535. If there is no app/src/main/java or app/src/main/kotlin then it creates a folder named "generated" and puts the connector into its own directory under there. If, however, one of those two directories do exist, then they are used. This is the desirable behavior. The other SDKs also go into a "generated" directory, so it made sense, to me, to put them all in there. Hopefully customers will change it anyways.

There must be some confusion here. The preference of using app/src/main/... is a special case that follows the conventional folder structure of basic Android apps. If you're going to remove this logic, please remove the comment above it as well.

I think the was some miscommunication here. AFAICT, we want to keep this logic - #7547 adds it back in.

Separately, even after that tweak, the output looks redundant/weird to me - by default, the connectorID shows up 2x in the output path

Discussed offline. Reverted that change.

Thanks for catching it!

dconeybe · 2024-08-09T01:31:29Z

src/init/features/dataconnect/sdk.ts

-      if (fs.existsSync(candidateDir)) {
-        baseDir = candidateDir;
-      }
-    }


There must be some confusion here. The preference of using app/src/main/... is a special case that follows the conventional folder structure of basic Android apps. If you're going to remove this logic, please remove the comment above it as well.

joehan

Not sure that we want this - lets sync up when we're all online

joehan

LGTM!

rosalyntan and others added 10 commits August 8, 2024 14:41

Bump FDC local toolkit to v1.3.3.

a75f2fc

Add PR number to changelog.

b7f1cb0

Merge branch 'master' into rosalyntan.bump

9d4f236

Merge remote-tracking branch 'origin/master' into fz/release

9052500

changelog

9178f70

bump

3801093

Update CHANGELOG.md

4308c66

m

714ceed

Update FDC SDK init defaults

a5c3099

merge

72e4a86

joehan approved these changes Aug 9, 2024

View reviewed changes

fredzqm commented Aug 9, 2024

View reviewed changes

fredzqm enabled auto-merge (squash) August 9, 2024 00:32

m

a998b4b

fredzqm disabled auto-merge August 9, 2024 00:42

PascalCase for swift

f1ca9ed

dconeybe requested changes Aug 9, 2024

View reviewed changes

joehan mentioned this pull request Aug 9, 2024

Tweak #7546 #7547

Closed

joehan requested changes Aug 9, 2024

View reviewed changes

fredzqm added 2 commits August 9, 2024 10:11

add <appDir>/app/src/main/kotlin logic back

e5978e7

default path

8d7b577

joehan approved these changes Aug 9, 2024

View reviewed changes

dconeybe approved these changes Aug 9, 2024

View reviewed changes

joehan merged commit 2f4e343 into master Aug 9, 2024

joehan deleted the fz/sdk-init-default branch August 9, 2024 17:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make FDC SDK init default outputDir disjoint #7546

Make FDC SDK init default outputDir disjoint #7546

Uh oh!

fredzqm commented Aug 9, 2024 •

edited

Loading

Uh oh!

fredzqm Aug 9, 2024

Uh oh!

joehan Aug 9, 2024

Uh oh!

fredzqm Aug 9, 2024 •

edited

Loading

Uh oh!

dconeybe Aug 9, 2024

Uh oh!

dconeybe Aug 9, 2024

Uh oh!

joehan Aug 9, 2024

Uh oh!

fredzqm Aug 9, 2024

Uh oh!

dconeybe Aug 9, 2024

Uh oh!

joehan left a comment

Uh oh!

joehan left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Make FDC SDK init default outputDir disjoint #7546

Make FDC SDK init default outputDir disjoint #7546

Uh oh!

Conversation

fredzqm commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

The existing behavior

Scenarios Tested

Uh oh!

fredzqm Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

joehan Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

fredzqm Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dconeybe Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

dconeybe Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

joehan Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

fredzqm Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

dconeybe Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

joehan left a comment

Choose a reason for hiding this comment

Uh oh!

joehan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fredzqm commented Aug 9, 2024 •

edited

Loading

fredzqm Aug 9, 2024 •

edited

Loading