Project typescript support#170
Draft
stevendborrelli wants to merge 6 commits into
Draft
Conversation
Signed-off-by: Steven Borrelli <steve@borrelli.org>
Signed-off-by: Steven Borrelli <steve@borrelli.org>
stevendborrelli
requested review from
a team,
jcogilvie and
tampakrap
as code owners
Contributor
📝 WalkthroughWalkthroughAdds end-to-end TypeScript support to the Crossplane CLI: a new ChangesTypeScript Support
Sequence DiagramsequenceDiagram
rect rgba(70, 130, 180, 0.5)
Note over Builder,SchemaManager: Schema generation
Builder->>DependencyManager: CollectSources(ctx, ch)
DependencyManager-->>Builder: []Source (xpkg resolved + versioned)
Builder->>SchemaManager: GenerateFromMultipleSources(allSources)
SchemaManager->>typescriptGenerator: GenerateFromCRD(mergedFS)
typescriptGenerator->>SchemaRunner: run crd-generate + tsc in node container
SchemaRunner-->>typescriptGenerator: compiled models/
typescriptGenerator-->>SchemaManager: output FS
SchemaManager->>SchemaManager: copy to schema repo, postProcess, update versions
end
rect rgba(60, 179, 113, 0.5)
Note over Builder,typescriptBuilder: Function build
Builder->>typescriptBuilder: Build(ctx, functionFS)
typescriptBuilder->>SchemaRunner: buildFunction (npm install + build in node container)
SchemaRunner-->>typescriptBuilder: /function tar
typescriptBuilder->>RuntimeImage: fetch distroless nodejs base per arch
typescriptBuilder->>RuntimeImage: append function layer + configureTypescriptImage
RuntimeImage-->>Builder: arch-specific OCI images
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 inconclusive)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
internal/project/functions/typescript.go (1)
59-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTiny doc nit: the comment says
npm cibut the build runsnpm install.The struct doc says we build via
npm ci, but both build scripts usenpm install(and the const comment even explains why). Mind tweaking the comment so it matches the actual behavior? Totally optional, just to avoid confusing future readers. 🙂📝 Suggested wording
-// A TypeScript embedded function is a full function-sdk-typescript project -// (package.json + src/). We build it by running npm ci and npm run build -// (which invokes tsgo) in a Node.js build container, then copy the dist/ -// and node_modules/ onto a distroless Node.js base. +// A TypeScript embedded function is a full function-sdk-typescript project +// (package.json + src/). We build it by running npm install and npm run build +// (which invokes tsgo) in a Node.js build container, then copy the dist/ +// and node_modules/ onto a distroless Node.js base.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/project/functions/typescript.go` around lines 59 - 64, The TypeScript builder doc comment is out of sync with the actual install step, since `typescriptBuilder` uses `npm install` rather than `npm ci`. Update the comment on `typescriptBuilder` (and any nearby related comment if needed) so it accurately describes the build flow: running `npm install` and `npm run build` in the Node.js build container before copying `dist/` and `node_modules/`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/crossplane/function/generate.go`:
- Around line 417-423: The `generate.go` schema probe is swallowing the
`afero.DirExists` failure on `schemasFS` under `typescript`, which can hide real
filesystem errors. Update the `hasSchemas` check in the schema-loading path to
capture and return the `DirExists` error instead of defaulting to “no schemas,”
and keep the subsequent `afero.ReadDir` handling in the same flow so failures
are propagated clearly from this code path.
In `@internal/project/functions/typescript.go`:
- Line 168: The `afero.DirExists` call is ignoring its error, so transient
filesystem failures can be mistaken for “no schemas” and silently skip the
schemas layer. Update the code around the `hasTSSchemas` check to handle and
propagate the `DirExists` error the same way the sibling `match` logic handles
`DirExists`/`Exists`, using the surrounding function that builds the TypeScript
schema detection flow.
- Around line 49-56: The package-level typescriptBuildScript constant is dead
code because buildFunction uses its own inline buildScript instead. Either
remove typescriptBuildScript from internal/project/functions/typescript.go if it
is no longer needed, or update buildFunction to reference typescriptBuildScript
so there is a single source of truth for the TypeScript build pipeline.
- Around line 166-210: The TypeScript build path still hardcodes the schemas
container location in the buildScript inside typescript.go, so it can miss
dependencies when c.SchemasPath changes. Update the logic in the function that
prepares schemasTar and constructs the container command to derive the
in-container schemas path from c.SchemasPath (using the same tsSchemasRel/base
path used for FSToTar and StartWithCopyFiles) instead of checking
/schemas/typescript, so the npm install branch follows the configured schemas
root.
- Around line 47-48: Update the TypeScript runtime base image constant used by
baseImageForArch from gcr.io/distroless/nodejs24-debian12 to the published
gcr.io/distroless/nodejs24-debian13 tag. Keep the change confined to the
typescriptRuntimeImage symbol in internal/project/functions/typescript.go so any
runtime image resolution uses the correct Node 24 distroless base.
In `@internal/schemas/generator/interface.go`:
- Line 46: The default AllLanguages() registry currently includes
typescriptGenerator, which makes TypeScript generation run for projects that did
not explicitly opt in. Update the schema language selection logic so TypeScript
is only added when schemas.languages explicitly includes "typescript" or when a
dedicated feature flag enables it, and keep the default generator set unchanged
for existing builds. Use the AllLanguages() function and &typescriptGenerator{}
as the main points to adjust.
In `@internal/schemas/generator/typescript.go`:
- Around line 219-227: stagedCRDPath currently flattens nested paths by
replacing "/" with "_" in the staged filename, which can make distinct source
paths collide and overwrite each other. Update stagedCRDPath to preserve path
uniqueness when generating the staged CRD name, using a collision-resistant
encoding of the original sourcePath (including directories) while still applying
the suffix/extension logic, and ensure any caller relying on staged TypeScript
CRD paths uses the updated naming consistently.
- Line 43: The TypeScript generator setup is currently pulling dependencies at
runtime with floating versions, which makes schema generation non-reproducible.
Update the generator flow around typescriptImage and the TypeScript generator
invocation to use pinned dependencies via a lockfile with npm ci, or bake
crd-generate and its dependencies into the image so the generation environment
is deterministic. Make sure any npm install usage and ^ version ranges are
removed or replaced with exact pinned versions.
In `@internal/schemas/manager/manager.go`:
- Around line 288-314: The merged source prefix in manager.go can collide
because sanitizeSourceID() may produce the same directory for different source
IDs, causing resources to overwrite during CopyFilesBetweenFs. Update the prefix
generation in the merge loop (and the matching logic in the other affected
block) to include a collision-resistant suffix such as the source index or a
stable hash derived from src.ID(), while keeping the existing source ID context.
Use the same prefix strategy wherever mergedFS/prefixedFS is built so each
source gets a unique namespace.
---
Nitpick comments:
In `@internal/project/functions/typescript.go`:
- Around line 59-64: The TypeScript builder doc comment is out of sync with the
actual install step, since `typescriptBuilder` uses `npm install` rather than
`npm ci`. Update the comment on `typescriptBuilder` (and any nearby related
comment if needed) so it accurately describes the build flow: running `npm
install` and `npm run build` in the Node.js build container before copying
`dist/` and `node_modules/`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4c74d371-9f20-491a-96b8-a83f8b3f9eb8
⛔ Files ignored due to path filters (5)
.github/renovate.json5is excluded by none and included by nonecmd/crossplane/function/templates/typescript/package.json.tmplis excluded by none and included by nonecmd/crossplane/function/templates/typescript/src/function.tsis excluded by none and included by nonecmd/crossplane/function/templates/typescript/src/main.tsis excluded by none and included by nonecmd/crossplane/function/templates/typescript/tsconfig.jsonis excluded by none and included by none
📒 Files selected for processing (11)
apis/dev/v1alpha1/project_types.gocmd/crossplane/function/generate.gocmd/crossplane/function/help/generate.mdcmd/crossplane/function/templates/typescript/README.mdinternal/dependency/manager.gointernal/project/build.gointernal/project/functions/build.gointernal/project/functions/typescript.gointernal/schemas/generator/interface.gointernal/schemas/generator/typescript.gointernal/schemas/manager/manager.go
Comment on lines
+417
to
+423
| hasSchemas, _ := afero.DirExists(c.schemasFS, "typescript") | ||
| if hasSchemas { | ||
| entries, err := afero.ReadDir(c.schemasFS, "typescript") | ||
| if err != nil { | ||
| return errors.Wrap(err, "cannot read typescript schemas directory") | ||
| } | ||
| hasSchemas = len(entries) > 0 |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Please propagate failures when probing schemas/typescript.
Could we return the afero.DirExists error here instead of treating it as “no schemas”? Right now an unreadable schemas directory silently generates a project without the local schema dependency, which makes the real failure much harder to diagnose later.
Suggested fix
func (c *generateCmd) generateTypescriptFiles(targetFS afero.Fs) error {
- hasSchemas, _ := afero.DirExists(c.schemasFS, "typescript")
+ hasSchemas, err := afero.DirExists(c.schemasFS, "typescript")
+ if err != nil {
+ return errors.Wrap(err, "cannot inspect typescript schemas directory")
+ }
if hasSchemas {
entries, err := afero.ReadDir(c.schemasFS, "typescript")
if err != nil {
return errors.Wrap(err, "cannot read typescript schemas directory")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| hasSchemas, _ := afero.DirExists(c.schemasFS, "typescript") | |
| if hasSchemas { | |
| entries, err := afero.ReadDir(c.schemasFS, "typescript") | |
| if err != nil { | |
| return errors.Wrap(err, "cannot read typescript schemas directory") | |
| } | |
| hasSchemas = len(entries) > 0 | |
| hasSchemas, err := afero.DirExists(c.schemasFS, "typescript") | |
| if err != nil { | |
| return errors.Wrap(err, "cannot inspect typescript schemas directory") | |
| } | |
| if hasSchemas { | |
| entries, err := afero.ReadDir(c.schemasFS, "typescript") | |
| if err != nil { | |
| return errors.Wrap(err, "cannot read typescript schemas directory") | |
| } | |
| hasSchemas = len(entries) > 0 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/crossplane/function/generate.go` around lines 417 - 423, The
`generate.go` schema probe is swallowing the `afero.DirExists` failure on
`schemasFS` under `typescript`, which can hide real filesystem errors. Update
the `hasSchemas` check in the schema-loading path to capture and return the
`DirExists` error instead of defaulting to “no schemas,” and keep the subsequent
`afero.ReadDir` handling in the same flow so failures are propagated clearly
from this code path.
Comment on lines
+47
to
+48
| // typescriptRuntimeImage is the distroless base used at runtime. | ||
| typescriptRuntimeImage = "gcr.io/distroless/nodejs24-debian12" |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
gcr.io/distroless/nodejs24 available debian tag latest
💡 Result:
The Distroless Node.js 24 image is available under the repository name gcr.io/distroless/nodejs24-debian13 [1][2]. You can use the tag latest with this image (e.g., gcr.io/distroless/nodejs24-debian13:latest) [2][3]. Other available tags for this image include nonroot, debug, and debug-nonroot [2][3]. These images are maintained by the GoogleContainerTools project and are updated regularly to include the latest stable versions of Node.js [1][4].
Citations:
- 1: https://github.com/GoogleContainerTools/distroless/blob/main/nodejs/README.md
- 2: https://github.com/GoogleContainerTools/distroless/blob/main/README.md
- 3: https://raw.githubusercontent.com/GoogleContainerTools/distroless/main/README.md
- 4: Bumping node archives to latest stable versions GoogleContainerTools/distroless#2046
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files internal/project/functions/typescript.go
wc -l internal/project/functions/typescript.go
sed -n '1,260p' internal/project/functions/typescript.go
rg -n "typescriptRuntimeImage|nodejs24-debian12|nodejs24-debian13|baseImageForArch" internal/project/functions -SRepository: crossplane/cli
Length of output: 9719
Switch the TypeScript runtime image to nodejs24-debian13
gcr.io/distroless/nodejs24-debian12 doesn’t appear to be published; the Node 24 distroless runtime is gcr.io/distroless/nodejs24-debian13, so this pull will fail when baseImageForArch tries to fetch the base image. Could this tag be updated?
🔧 Proposed change
- typescriptRuntimeImage = "gcr.io/distroless/nodejs24-debian12"
+ typescriptRuntimeImage = "gcr.io/distroless/nodejs24-debian13"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // typescriptRuntimeImage is the distroless base used at runtime. | |
| typescriptRuntimeImage = "gcr.io/distroless/nodejs24-debian12" | |
| // typescriptRuntimeImage is the distroless base used at runtime. | |
| typescriptRuntimeImage = "gcr.io/distroless/nodejs24-debian13" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/project/functions/typescript.go` around lines 47 - 48, Update the
TypeScript runtime base image constant used by baseImageForArch from
gcr.io/distroless/nodejs24-debian12 to the published
gcr.io/distroless/nodejs24-debian13 tag. Keep the change confined to the
typescriptRuntimeImage symbol in internal/project/functions/typescript.go so any
runtime image resolution uses the correct Node 24 distroless base.
Comment on lines
+49
to
+56
| // typescriptBuildScript is the shell pipeline that runs in the build | ||
| // container. Installs dependencies and compiles TypeScript using tsgo. | ||
| // We use npm install instead of npm ci because the schemas package may | ||
| // be added dynamically and the lock file won't be in sync. | ||
| typescriptBuildScript = `set -eu | ||
| npm install --no-fund | ||
| npm run build | ||
| ` |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '\btypescriptBuildScript\b' --type=goRepository: crossplane/cli
Length of output: 152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## internal/project/functions/typescript.go (relevant slice)\n'
sed -n '1,260p' internal/project/functions/typescript.go
printf '\n## references to buildScript in the same file\n'
rg -n 'buildScript|typescriptBuildScript' internal/project/functions/typescript.goRepository: crossplane/cli
Length of output: 8950
Remove the unused typescriptBuildScript constant internal/project/functions/typescript.go:49
buildFunction already defines its own inline buildScript, so this package-level const looks like dead code. Should we drop it, or point buildFunction at it so there’s a single source of truth?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/project/functions/typescript.go` around lines 49 - 56, The
package-level typescriptBuildScript constant is dead code because buildFunction
uses its own inline buildScript instead. Either remove typescriptBuildScript
from internal/project/functions/typescript.go if it is no longer needed, or
update buildFunction to reference typescriptBuildScript so there is a single
source of truth for the TypeScript build pipeline.
Comment on lines
+166
to
+210
| tsSchemasRel := path.Join(c.SchemasPath, "typescript") | ||
| tsSchemasFS := afero.NewBasePathFs(c.ProjectFS, tsSchemasRel) | ||
| hasTSSchemas, _ := afero.DirExists(tsSchemasFS, ".") | ||
| var schemasTar []byte | ||
| if hasTSSchemas { | ||
| schemasTar, err = filesystem.FSToTar(tsSchemasFS, tsSchemasRel) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "failed to tar typescript schemas") | ||
| } | ||
| } | ||
|
|
||
| buildImage := b.buildImage | ||
| _, rewritten, err := b.configStore.RewritePath(ctx, b.buildImage) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "failed to rewrite build image") | ||
| } | ||
| if rewritten != "" { | ||
| buildImage = rewritten | ||
| } | ||
|
|
||
| // Build script that: | ||
| // 1. Runs npm install and build in the function's original path (so relative deps resolve) | ||
| // 2. Copies the built artifacts to /function for the runtime layer | ||
| fnPath := "/" + filepath.ToSlash(c.FunctionPath) | ||
| buildScript := `set -eu | ||
| # First, install dependencies for the schemas package so TypeScript can resolve the base types | ||
| if [ -d "/schemas/typescript" ] && [ -f "/schemas/typescript/package.json" ]; then | ||
| cd /schemas/typescript && npm install --no-fund | ||
| cd - | ||
| fi | ||
| npm install --no-fund | ||
| npm run build | ||
| # Use -L to dereference symlinks so file: dependencies (like crossplane-models) | ||
| # are copied as actual files, not symlinks that won't resolve at runtime. | ||
| cp -rL . /function | ||
| ` | ||
|
|
||
| opts := []docker.StartContainerOption{ | ||
| docker.StartWithCopyFiles(fnTar, "/"), | ||
| docker.StartWithCommand([]string{"sh", "-c", buildScript}), | ||
| docker.StartWithWorkingDirectory(fnPath), | ||
| } | ||
| if schemasTar != nil { | ||
| opts = append(opts, docker.StartWithCopyFiles(schemasTar, "/")) | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find how SchemasPath is defined/populated on BuildContext.
rg -nP '\bSchemasPath\b' --type=go -C3Repository: crossplane/cli
Length of output: 152
🏁 Script executed:
#!/bin/bash
set -eu
# Map likely files and search for BuildContext, SchemasPath, and tar copy paths.
git ls-files | rg '(^|/)(.*typescript.*\.go|.*project.*\.go|.*context.*\.go|.*build.*\.go)$' || true
printf '\n## SchemasPath references\n'
rg -n --type=go 'SchemasPath|schemasPath|Schemas path|schema(s)?Path' .
printf '\n## BuildContext references\n'
rg -n --type=go 'type .*BuildContext|struct .*BuildContext|BuildContext' .
printf '\n## filesystem tar/copy usage\n'
rg -n --type=go 'FSToTar|StartWithCopyFiles|NewBasePathFs|StartWithWorkingDirectory|RewritePath' internal .Repository: crossplane/cli
Length of output: 22859
🏁 Script executed:
#!/bin/bash
set -eu
# Inspect the SchemasPath field definition and any defaulting.
sed -n '1,220p' internal/project/functions/build.go
printf '\n---\n'
sed -n '1,240p' apis/dev/v1alpha1/project_types.go
printf '\n---\n'
sed -n '520,580p' internal/project/build.go
printf '\n---\n'
sed -n '150,220p' internal/project/functions/typescript.go
printf '\n---\n'
sed -n '150,220p' internal/project/functions/python.goRepository: crossplane/cli
Length of output: 22219
Keep the schemas root dynamic
c.SchemasPath is configurable, but the container script still checks /schemas/typescript. If a project moves the schemas root, the npm install branch is skipped and the TypeScript build can miss the base types. Could this derive from c.SchemasPath instead of hardcoding /schemas?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/project/functions/typescript.go` around lines 166 - 210, The
TypeScript build path still hardcodes the schemas container location in the
buildScript inside typescript.go, so it can miss dependencies when c.SchemasPath
changes. Update the logic in the function that prepares schemasTar and
constructs the container command to derive the in-container schemas path from
c.SchemasPath (using the same tsSchemasRel/base path used for FSToTar and
StartWithCopyFiles) instead of checking /schemas/typescript, so the npm install
branch follows the configured schemas root.
| // the relative path in package.json (e.g., "file:../../schemas/typescript"). | ||
| tsSchemasRel := path.Join(c.SchemasPath, "typescript") | ||
| tsSchemasFS := afero.NewBasePathFs(c.ProjectFS, tsSchemasRel) | ||
| hasTSSchemas, _ := afero.DirExists(tsSchemasFS, ".") |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Swallowed error on afero.DirExists.
The sibling match method checks the DirExists/Exists errors, but here the error is dropped. A transient FS error would be misread as "no schemas," silently skipping the schemas layer rather than failing loudly. Want to propagate it for consistency?
🛡️ Suggested change
- hasTSSchemas, _ := afero.DirExists(tsSchemasFS, ".")
+ hasTSSchemas, err := afero.DirExists(tsSchemasFS, ".")
+ if err != nil {
+ return nil, errors.Wrap(err, "failed to check for typescript schemas")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| hasTSSchemas, _ := afero.DirExists(tsSchemasFS, ".") | |
| hasTSSchemas, err := afero.DirExists(tsSchemasFS, ".") | |
| if err != nil { | |
| return nil, errors.Wrap(err, "failed to check for typescript schemas") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/project/functions/typescript.go` at line 168, The `afero.DirExists`
call is ignoring its error, so transient filesystem failures can be mistaken for
“no schemas” and silently skip the schemas layer. Update the code around the
`hasTSSchemas` check to handle and propagate the `DirExists` error the same way
the sibling `match` logic handles `DirExists`/`Exists`, using the surrounding
function that builds the TypeScript schema detection flow.
| &jsonGenerator{}, | ||
| &kclGenerator{}, | ||
| &pythonGenerator{}, | ||
| &typescriptGenerator{}, |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Keep TypeScript generation opt-in until the new build dependency is intentional.
Could we avoid adding this generator to the default AllLanguages() set unless the project explicitly requests schemas.languages: ["typescript"] or a feature flag enables it? Projects that omit schemas.languages currently generate every registered language, so this can make existing builds pull Node and run npm install unexpectedly. As per coding guidelines, “Implement feature flags for all new experimental features affecting apis/** or features that significantly affect behavior.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/schemas/generator/interface.go` at line 46, The default
AllLanguages() registry currently includes typescriptGenerator, which makes
TypeScript generation run for projects that did not explicitly opt in. Update
the schema language selection logic so TypeScript is only added when
schemas.languages explicitly includes "typescript" or when a dedicated feature
flag enables it, and keep the default generator set unchanged for existing
builds. Use the AllLanguages() function and &typescriptGenerator{} as the main
points to adjust.
Source: Coding guidelines
| typescriptModelsFolder = "models" | ||
| // typescriptImage is the Docker image used to run crd-generate. | ||
| // We use a Node.js image and install the tool at runtime. | ||
| typescriptImage = "docker.io/library/node:22-slim" |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File outline ==\n'
ast-grep outline internal/schemas/generator/typescript.go --view expanded || true
printf '\n== Relevant lines ==\n'
sed -n '1,140p' internal/schemas/generator/typescript.go
printf '\n== Later referenced lines ==\n'
sed -n '280,330p' internal/schemas/generator/typescript.go
printf '\n== Search for npm/install/version handling ==\n'
rg -n --hidden --glob '!**/vendor/**' 'npm install|npm ci|package-lock|yarn.lock|pnpm-lock|typescriptImage|node:22-slim|^\\s*\\^' internal/schemas/generator .github . || trueRepository: crossplane/cli
Length of output: 7784
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File outline ==\n'
ast-grep outline internal/schemas/generator/typescript.go --view expanded || true
printf '\n== Relevant lines ==\n'
sed -n '1,140p' internal/schemas/generator/typescript.go
printf '\n== Later referenced lines ==\n'
sed -n '280,330p' internal/schemas/generator/typescript.go
printf '\n== Search for npm/install/version handling ==\n'
rg -n --hidden --glob '!**/vendor/**' 'npm install|npm ci|package-lock|yarn.lock|pnpm-lock|typescriptImage|node:22-slim|^\s*\^' internal/schemas/generator .github . || trueRepository: crossplane/cli
Length of output: 7784
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "hello"Repository: crossplane/cli
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== TypeScript generator call sites ==\n'
rg -n --hidden --glob '!**/vendor/**' 'typescriptGenerator|GenerateFromCRD\(|SchemaLanguageTypescript|generate.*typescript|typescript schema|crossplane-models' .
printf '\n== Surrounding lines near the generator execution ==\n'
sed -n '232,330p' internal/schemas/generator/typescript.goRepository: crossplane/cli
Length of output: 7010
Pin the TypeScript generator dependencies Could we replace the runtime npm install and ^ ranges with a lockfile plus npm ci, or bake crd-generate into the image? The current setup can drift as upstream packages change, which makes the generated schemas non-reproducible.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/schemas/generator/typescript.go` at line 43, The TypeScript
generator setup is currently pulling dependencies at runtime with floating
versions, which makes schema generation non-reproducible. Update the generator
flow around typescriptImage and the TypeScript generator invocation to use
pinned dependencies via a lockfile with npm ci, or bake crd-generate and its
dependencies into the image so the generation environment is deterministic. Make
sure any npm install usage and ^ version ranges are removed or replaced with
exact pinned versions.
Comment on lines
+219
to
+227
| func stagedCRDPath(sourcePath, suffix string) string { | ||
| clean := filepath.ToSlash(filepath.Clean(sourcePath)) | ||
| clean = strings.TrimPrefix(clean, "./") | ||
| clean = strings.TrimPrefix(clean, "/") | ||
| if suffix != "" { | ||
| ext := filepath.Ext(clean) | ||
| clean = strings.TrimSuffix(clean, ext) + "-" + suffix + ext | ||
| } | ||
| return strings.ReplaceAll(clean, "/", "_") |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make staged CRD filenames collision-resistant.
Thanks for staging generated files before the TypeScript pass. One edge case: stagedCRDPath() flattens directories with _, so distinct paths like a/b_c.yaml and a_b/c.yaml both become a_b_c.yaml and one CRD can overwrite the other.
Suggested direction
func stagedCRDPath(sourcePath, suffix string) string {
clean := filepath.ToSlash(filepath.Clean(sourcePath))
clean = strings.TrimPrefix(clean, "./")
clean = strings.TrimPrefix(clean, "/")
+ // Add a stable hash of the original clean path so flattened names do not collide.
+ sum := sha256.Sum256([]byte(clean))
+ hash := hex.EncodeToString(sum[:])[:12]
if suffix != "" {
ext := filepath.Ext(clean)
clean = strings.TrimSuffix(clean, ext) + "-" + suffix + ext
}
- return strings.ReplaceAll(clean, "/", "_")
+ ext := filepath.Ext(clean)
+ flat := strings.ReplaceAll(strings.TrimSuffix(clean, ext), "/", "_")
+ return flat + "-" + hash + ext
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func stagedCRDPath(sourcePath, suffix string) string { | |
| clean := filepath.ToSlash(filepath.Clean(sourcePath)) | |
| clean = strings.TrimPrefix(clean, "./") | |
| clean = strings.TrimPrefix(clean, "/") | |
| if suffix != "" { | |
| ext := filepath.Ext(clean) | |
| clean = strings.TrimSuffix(clean, ext) + "-" + suffix + ext | |
| } | |
| return strings.ReplaceAll(clean, "/", "_") | |
| func stagedCRDPath(sourcePath, suffix string) string { | |
| clean := filepath.ToSlash(filepath.Clean(sourcePath)) | |
| clean = strings.TrimPrefix(clean, "./") | |
| clean = strings.TrimPrefix(clean, "/") | |
| // Add a stable hash of the original clean path so flattened names do not collide. | |
| sum := sha256.Sum256([]byte(clean)) | |
| hash := hex.EncodeToString(sum[:])[:12] | |
| if suffix != "" { | |
| ext := filepath.Ext(clean) | |
| clean = strings.TrimSuffix(clean, ext) + "-" + suffix + ext | |
| } | |
| ext := filepath.Ext(clean) | |
| flat := strings.ReplaceAll(strings.TrimSuffix(clean, ext), "/", "_") | |
| return flat + "-" + hash + ext | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/schemas/generator/typescript.go` around lines 219 - 227,
stagedCRDPath currently flattens nested paths by replacing "/" with "_" in the
staged filename, which can make distinct source paths collide and overwrite each
other. Update stagedCRDPath to preserve path uniqueness when generating the
staged CRD name, using a collision-resistant encoding of the original sourcePath
(including directories) while still applying the suffix/extension logic, and
ensure any caller relying on staged TypeScript CRD paths uses the updated naming
consistently.
Comment on lines
+288
to
+314
| for _, src := range sources { | ||
| version, err := src.Version(ctx) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "failed to get version for source %s", src.ID()) | ||
| } | ||
|
|
||
| // Check if this source is already up to date | ||
| existing, err := m.currentVersion(src.ID()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if existing == version { | ||
| // Source is up to date, but we still need to include its resources | ||
| // for the merged generation to work correctly | ||
| } | ||
|
|
||
| srcFS, err := src.Resources(ctx) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "failed to get resources for source %s", src.ID()) | ||
| } | ||
|
|
||
| // Copy resources into merged filesystem under a unique prefix | ||
| // to avoid file name collisions | ||
| prefix := sanitizeSourceID(src.ID()) | ||
| prefixedFS := afero.NewBasePathFs(mergedFS, prefix) | ||
| if err := filesystem.CopyFilesBetweenFs(srcFS, prefixedFS); err != nil { | ||
| return errors.Wrapf(err, "failed to copy resources from source %s", src.ID()) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Prevent sanitized source ID collisions when merging sources.
Nice approach using per-source prefixes. Could we include the source index or a stable hash in the prefix? sanitizeSourceID() can map distinct IDs to the same directory, causing one source’s CRDs to overwrite or merge with another before generation.
Suggested localized fix
- for _, src := range sources {
+ for i, src := range sources {
version, err := src.Version(ctx)
if err != nil {
return errors.Wrapf(err, "failed to get version for source %s", src.ID())
}
@@
- prefix := sanitizeSourceID(src.ID())
+ prefix := fmt.Sprintf("%04d_%s", i, sanitizeSourceID(src.ID()))
prefixedFS := afero.NewBasePathFs(mergedFS, prefix)Also applies to: 390-397
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/schemas/manager/manager.go` around lines 288 - 314, The merged
source prefix in manager.go can collide because sanitizeSourceID() may produce
the same directory for different source IDs, causing resources to overwrite
during CopyFilesBetweenFs. Update the prefix generation in the merge loop (and
the matching logic in the other affected block) to include a collision-resistant
suffix such as the source index or a stable hash derived from src.ID(), while
keeping the existing source ID context. Use the same prefix strategy wherever
mergedFS/prefixedFS is built so each source gets a unique namespace.
Signed-off-by: Steven Borrelli <steve@borrelli.org>
Signed-off-by: Steven Borrelli <steve@borrelli.org>
Signed-off-by: Steven Borrelli <steve@borrelli.org>
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
1 participant
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.
Add TypeScript support for composition functions
Fixes #169
Description
This PR adds first-class TypeScript support to the Crossplane CLI, enabling developers to write composition functions in TypeScript with full type safety. An example project using this branch is https://github.com/stevendborrelli/configuration-aws-network-ts-xp-cli.
Changes
TypeScript Function Builder (
internal/project/functions/typescript.go)package.json+src/directorynode:24-slim)npm installandnpm run build(typically invokingtsgo)gcr.io/distroless/nodejs24-debian12)crossplane-modelsschema package as afile:dependencyTypeScript Schema Generation (
internal/schemas/generator/typescript.go)@kubernetes-models/crd-generate@kubernetes-models/basecrossplane-modelsnpm package that functions can importcrossplane-models/ec2.aws.upbound.io/v1beta1Merged Schema Generation (
internal/schemas/manager/manager.go,internal/project/build.go)GenerateFromMultipleSources()to generate schemas from all CRD sources in a single passindex.jswith proper cross-referencesFunction Template (
cmd/crossplane/function/generate.go)crossplane function init --language=typescriptpackage.jsonwith SDK dependenciestsconfig.json)Example Usage
Generated Package Structure
Dependencies
This feature builds on existing ecosystem work:
@crossplane-org/function-sdk-typescript- Official TypeScript SDK for composition functions@kubernetes-models/crd-generate- Generates TypeScript classes from CRDs with constructors, interfaces, and runtime validationtsgo- Fast TypeScript compiler for build pipelinesTesting
crossplane project buildbuilds TypeScript functionscrossplane function init --language=typescriptcreates a working templateBreaking Changes
None. This is additive functionality.
Checklist
I have:
./nix.sh flake checkto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.