TypeScript based charts definition. Closes #54 #502
Conversation
Introduce nelm chart create integration that can generate TypeScript-based charts. Add --only-ts flag, tschart package to generate boilerplate and Chart.yaml/values/.helmignore, register the new command
| }, | ||
| ) | ||
|
|
||
| afterAllCommandsBuiltFuncs[cmd] = func(cmd *cobra.Command) error { |
There was a problem hiding this comment.
LogLevel LogColorMode TempDirPath are missing
cmd/nelm/chart_init.go
Outdated
| chartPath = args[0] | ||
| } | ||
|
|
||
| // Convert to absolute path for reliable name extraction |
There was a problem hiding this comment.
Move the code from here to below to a separate action
There was a problem hiding this comment.
Also, move cfg.TS in action Options too
cmd/nelm/chart_init.go
Outdated
| }, | ||
| }, | ||
| func(cmd *cobra.Command, args []string) error { | ||
| ctx = log.SetupLogging(ctx, log.InfoLevel, log.SetupLoggingOptions{}) |
There was a problem hiding this comment.
Do it like here (except don't set LogIsParseable):
Lines 48 to 51 in 675f5d9
internal/tschart/transformer.go
Outdated
| if _, err := os.Stat(epPath); err == nil { | ||
| return ep | ||
| } |
There was a problem hiding this comment.
You should hard fail on any unexpected error, like permission denied. "" should be returned only if there is no file.
There was a problem hiding this comment.
I see you use os.Stat in similar fashion in many other places. Redo it there too.
| ) | ||
|
|
||
| var ( | ||
| EntryPoints = []string{"src/index.ts", "src/index.js"} |
There was a problem hiding this comment.
Can't we reliably get it from package.json or from somewhere? If not, don't bother
There was a problem hiding this comment.
We have 2 places where src can be defined:
- for typescript we can specify only source files https://www.typescriptlang.org/tsconfig/#include. We can try to guest from source files what should be picked
- in package.json there are sections that can describe entry points, but only for already compiled files or for projects that are plain javascript
I propose to keep current approach and document it
| tsEntries := ` | ||
| # TypeScript chart files | ||
| ts/node_modules/ | ||
| ts/dist/ | ||
| ` |
There was a problem hiding this comment.
Reuse, already defined in another place
internal/tschart/init.go
Outdated
| }{ | ||
| {filepath.Join(srcDir, "index.ts"), generateIndexTS()}, | ||
| {filepath.Join(srcDir, "helpers.ts"), generateHelpersTS()}, | ||
| {filepath.Join(srcDir, "resources.ts"), generateResourcesTS()}, |
There was a problem hiding this comment.
Can we pls split this into deployment.ts, service.ts? Like people usually do with helm templates
internal/tschart/init.go
Outdated
| {filepath.Join(srcDir, "index.ts"), generateIndexTS()}, | ||
| {filepath.Join(srcDir, "helpers.ts"), generateHelpersTS()}, | ||
| {filepath.Join(srcDir, "resources.ts"), generateResourcesTS()}, | ||
| {filepath.Join(typesDir, "nelm.d.ts"), generateNelmDTS()}, |
There was a problem hiding this comment.
I guess that's temporary... Or it shouldn't be? Since we only want types in nelm SDK, at least for now, maybe it's gonna be easier to just generate the types and put them here...
The issue is how we gonna manage backwards compatibility. If some field is added to, let's say, Capabilities, what we do?
There was a problem hiding this comment.
I think there are should be a separate npm-package for nelm and we can validate in nelm that version is compatible with currently used package. Or if we support backwards compatibility, we can allow user to use outdated types definitions, if user wants to have have actual types it can update package from npm.
Another path - is keep this as it allow to regenerate this file by some command.
I think we should implement as proper package
There was a problem hiding this comment.
I guess you are right, let's go with the separate package. And if we provide some helpers or abstractions libraries down the line, we would still need to make separate packages for this.
internal/tschart/init.go
Outdated
| for _, f := range files { | ||
| if _, err := os.Stat(f.path); err == nil { | ||
| return fmt.Errorf("TypeScript file already exists: %s. Cannot initialize in a directory with existing TypeScript chart files", f.path) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Let's just check if ts directory exists. If it does, then fail
internal/tschart/init.go
Outdated
| {filepath.Join(typesDir, "nelm.d.ts"), generateNelmDTS()}, | ||
| {filepath.Join(tsDir, "tsconfig.json"), generateTSConfig()}, | ||
| {filepath.Join(tsDir, "package.json"), generatePackageJSON(chartName)}, | ||
| {filepath.Join(tsDir, ".gitignore"), generateTSGitignore()}, |
There was a problem hiding this comment.
Can we move it a level up? To the chart root. Handle it like we do with .helmignore
There was a problem hiding this comment.
Wait, this already happens in tschart.EnsureGitignore(). We probably should just remove this line from here.
| ) | ||
|
|
||
| const ( | ||
| TSSourceDir = "ts/" |
There was a problem hiding this comment.
Having this as a constant is good. But would be great to use this constant in every place where ts dir is specified: in many places it is still hardcoded
internal/tschart/transformer.go
Outdated
|
|
||
| entrypointFile := findEntrypoint(tsDir) | ||
| if entrypointFile == "" { | ||
| log.Default.Debug(ctx, "No TypeScript entrypoint found, skipping transformation") |
internal/tschart/transformer.go
Outdated
|
|
||
| nodeModulesPath := filepath.Join(tsDir, "node_modules") | ||
| if _, err := os.Stat(nodeModulesPath); os.IsNotExist(err) { | ||
| log.Default.Debug(ctx, "No node_modules directory found, skipping vendor bundle") |
There was a problem hiding this comment.
We should handle these cases:
- node_modules are up-to-date: proceed
- node_modules are not-up-to-date: error
- node_modules are empty/nonexistent, but there are dependencies in the project: error
- node_modules are empty/nonexistent, but there are no dependencies in the project: proceed
- node_modules are not empty, but there are no dependencies: error
Not sure how can we do it. And can we do this reliably and programmatically at all.
internal/tschart/transformer.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (t *Transformer) TransformChartForRender(ctx context.Context, chartPath string, chart *helmchart.Chart) error { |
There was a problem hiding this comment.
I guess this whole function is useless at this point? There is no transformation, just a few debug statements. I guess you should merge this code with some other function called later (if there is anything useful left here).
There was a problem hiding this comment.
Although it would be good to somehow detect broken ts chart and fail. What can we check? Maybe error if:
- There are deps, but node_modules and vendor/libs.js are non-existent/empty
- No entrypoint file
Also, if we validate it once early, we don't need to constantly check for deps and entrypoint every time.
internal/tschart/transformer.go
Outdated
| return "", nil, nil | ||
| } | ||
|
|
||
| if _, err := os.Stat(nodeModulesPath); err == nil { |
There was a problem hiding this comment.
Can we also check for its emptiness, and handle it like it doesn't exist if empty?
internal/tschart/engine.go
Outdated
| var appBundle string | ||
| var err error | ||
|
|
||
| if isLocalDir { |
There was a problem hiding this comment.
I'm not sure I understood what we are doing here.
chart.RuntimeFiles (and maybe some in chart.Files?) should be populated with all we need, we don't need to go to filesystem for this. You don't wanna work with FS, believe me: remote vs local charts, dependencies in chart tree as archives, as directories, or cached somewhere in .werf, etc. It's complex, and all of this is handled in load.Load(), where we build the Chart{}. So just use chart struct.
Also, we need to handle ts from subcharts in the same manner, are we doing it? I don't see where.
The logic should be like this: get all the files you need from chart.RuntimeFiles and render ts for the chart, then go to the next chart via chart.Dependencies() and do the same thing for it. At the end concatenate all results from all the charts/subcharts.
| func buildRenderContext(renderedValues chartutil.Values, chart *helmchart.Chart) map[string]interface{} { | ||
| renderContext := renderedValues.AsMap() | ||
|
|
||
| if valuesInterface, ok := renderContext["Values"]; ok { |
There was a problem hiding this comment.
Some things are missing: Capabilities, Chart, Release, Runtime (this one is nelm-specific). They were already computed somewhere, we just need to pass them here
There was a problem hiding this comment.
they are present i tested it with:
import { RenderContext, RenderResult } from '../types/nelm';
export function render($: RenderContext): RenderResult {
const debugInfo = {
apiVersion: 'v1',
kind: 'ConfigMap',
metadata: {
name: 'render-context-debug',
},
data: {
'values': JSON.stringify($.Values, null, 2),
'release': JSON.stringify($.Release, null, 2),
'chart': JSON.stringify($.Chart, null, 2),
'capabilities': JSON.stringify($.Capabilities, null, 2),
'runtime': JSON.stringify(($ as any).Runtime, null, 2),
'files-keys': JSON.stringify(Object.keys($.Files || {}), null, 2),
},
};
return { manifests: [debugInfo] };
}here is output(NELM_FEAT_TYPESCRIPT=true ./bin/nelm chart render --log-level=debug ../nelm-ts-charts/ts-test-chart/ --set-runtime-json=a=1):
---
# Source: ts-test-chart/ts/src/index.ts
apiVersion: v1
data:
capabilities: |-
{
"KubeVersion": {
"Version": "v1.20.0",
"Major": "1",
"Minor": "20"
},
"APIVersions": [
"v1",
"admissionregistration.k8s.io/v1",
"admissionregistration.k8s.io/v1alpha1",
"admissionregistration.k8s.io/v1beta1",
"internal.apiserver.k8s.io/v1alpha1",
"apps/v1",
"apps/v1beta1",
"apps/v1beta2",
"authentication.k8s.io/v1",
"authentication.k8s.io/v1alpha1",
"authentication.k8s.io/v1beta1",
"authorization.k8s.io/v1",
"authorization.k8s.io/v1beta1",
"autoscaling/v1",
"autoscaling/v2",
"autoscaling/v2beta1",
"autoscaling/v2beta2",
"batch/v1",
"batch/v1beta1",
"certificates.k8s.io/v1",
"certificates.k8s.io/v1beta1",
"certificates.k8s.io/v1alpha1",
"coordination.k8s.io/v1beta1",
"coordination.k8s.io/v1",
"discovery.k8s.io/v1",
"discovery.k8s.io/v1beta1",
"events.k8s.io/v1",
"events.k8s.io/v1beta1",
"extensions/v1beta1",
"flowcontrol.apiserver.k8s.io/v1",
"flowcontrol.apiserver.k8s.io/v1beta1",
"flowcontrol.apiserver.k8s.io/v1beta2",
"flowcontrol.apiserver.k8s.io/v1beta3",
"networking.k8s.io/v1",
"networking.k8s.io/v1alpha1",
"networking.k8s.io/v1beta1",
"node.k8s.io/v1",
"node.k8s.io/v1alpha1",
"node.k8s.io/v1beta1",
"policy/v1",
"policy/v1beta1",
"rbac.authorization.k8s.io/v1",
"rbac.authorization.k8s.io/v1beta1",
"rbac.authorization.k8s.io/v1alpha1",
"resource.k8s.io/v1alpha2",
"scheduling.k8s.io/v1alpha1",
"scheduling.k8s.io/v1beta1",
"scheduling.k8s.io/v1",
"storage.k8s.io/v1beta1",
"storage.k8s.io/v1",
"storage.k8s.io/v1alpha1",
"apiextensions.k8s.io/v1beta1",
"apiextensions.k8s.io/v1"
],
"HelmVersion": {
"Version": "v3.14",
"GitCommit": "",
"GitTreeState": "",
"GoVersion": "go1.24.10"
}
}
chart: |-
{
"Version": "0.1.0",
"Description": "",
"Sources": [],
"APIVersion": "v2",
"Condition": "",
"Type": "",
"Name": "ts-test-chart",
"AppVersion": "",
"Keywords": [],
"Home": "",
"Icon": "",
"Tags": "",
"Annotations": {}
}
files-keys: |-
[
".helmignore"
]
release: |-
{
"IsUpgrade": false,
"IsInstall": true,
"Revision": 1,
"Service": "Helm",
"Name": "stub-release",
"Namespace": "stub-namespace"
}
runtime: |-
{
"a": 1
}
values: |-
{
"testValue": "hello"
}
kind: ConfigMap
metadata:
annotations: {}
labels: {}
name: render-context-debug
internal/tschart/engine.go
Outdated
| "sigs.k8s.io/yaml" | ||
| ) | ||
|
|
||
| const OutputFile = "ts/render_output.yaml" |
There was a problem hiding this comment.
Should be something like this:
<chart-path>/ts/<path-to-entrypoint>
Where chart-path is dynamically computed, and can be something like rootchart/subchart/subchart (you can get this info from Chart{} and Chart{}.Dependencies()).
path-to-entrypoint should also be dynamically computed, like we do in all the other places with entrypoint.
internal/tschart/helpers/console.go
Outdated
| "github.com/werf/nelm/pkg/log" | ||
| ) | ||
|
|
||
| func SetupConsoleGlobal(ctx context.Context, runtime *goja.Runtime) { |
There was a problem hiding this comment.
Hey, how about we use something like github.com/dop251/goja_nodejs/ instead? I really don't want to maintain something like this by myself. Inject whatever you seem useful from goja_nodejs lib into goja runtime, so it behaves like nodejs?
internal/tschart/transformer.go
Outdated
| if fileExists(basePath) { | ||
| return basePath | ||
| } | ||
| for _, ext := range []string{".ts", ".tsx", ".js", ".jsx", ".json"} { |
There was a problem hiding this comment.
Here and in a few other places I see these hardcoded extensions. What are we doing here with this virtual-fs?
Co-authored-by: Ilya Lesikov <[email protected]>
|
@ilya-lesikov I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 23 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="internal/tschart/transformer_mem.go">
<violation number="1" location="internal/tschart/transformer_mem.go:217">
P1: Using `filepath.Join` in virtual filesystem context will cause cross-platform issues. On Windows, this produces backslash-separated paths that won't match the forward-slash keys in the files map. Use `path.Join` from the `path` package instead, which always uses forward slashes.</violation>
</file>
<file name="internal/tschart/init.go">
<violation number="1" location="internal/tschart/init.go:331">
P2: The `chartName` is directly interpolated into JSON without escaping. If the chart name contains special JSON characters (quotes, backslashes), this produces invalid JSON. Consider using `json.Marshal` for proper escaping or validate the chart name before use.</violation>
</file>
<file name="internal/tschart/runtime.go">
<violation number="1" location="internal/tschart/runtime.go:104">
P2: Calling `ToObject(nil)` with nil runtime is unsafe and could cause a panic. The `formatJSError` function needs access to the goja runtime to safely convert the error value to an object. Consider passing the `*goja.Runtime` as a parameter, or add defensive nil checks.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
| } | ||
|
|
||
| func generatePackageJSON(chartName string) string { | ||
| return fmt.Sprintf(`{ |
There was a problem hiding this comment.
P2: The chartName is directly interpolated into JSON without escaping. If the chart name contains special JSON characters (quotes, backslashes), this produces invalid JSON. Consider using json.Marshal for proper escaping or validate the chart name before use.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/tschart/init.go, line 331:
<comment>The `chartName` is directly interpolated into JSON without escaping. If the chart name contains special JSON characters (quotes, backslashes), this produces invalid JSON. Consider using `json.Marshal` for proper escaping or validate the chart name before use.</comment>
<file context>
@@ -0,0 +1,386 @@
+}
+
+func generatePackageJSON(chartName string) string {
+ return fmt.Sprintf(`{
+ "name": "%s",
+ "version": "0.1.0",
</file context>
|
|
||
| errMsg := gojaErr.Error() | ||
|
|
||
| stackProp := gojaErr.Value().ToObject(nil).Get("stack") |
There was a problem hiding this comment.
P2: Calling ToObject(nil) with nil runtime is unsafe and could cause a panic. The formatJSError function needs access to the goja runtime to safely convert the error value to an object. Consider passing the *goja.Runtime as a parameter, or add defensive nil checks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/tschart/runtime.go, line 104:
<comment>Calling `ToObject(nil)` with nil runtime is unsafe and could cause a panic. The `formatJSError` function needs access to the goja runtime to safely convert the error value to an object. Consider passing the `*goja.Runtime` as a parameter, or add defensive nil checks.</comment>
<file context>
@@ -0,0 +1,112 @@
+
+ errMsg := gojaErr.Error()
+
+ stackProp := gojaErr.Value().ToObject(nil).Get("stack")
+ if stackProp == nil || goja.IsUndefined(stackProp) || goja.IsNull(stackProp) {
+ return fmt.Errorf("%s\n at %s", errMsg, currentFile)
</file context>
|
/review |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #aee90b
Actionable Suggestions - 4
-
ts/nelm-types/src/index.ts - 1
- Type mismatch in Tags field · Line 28-28
-
internal/chart/chart_render.go - 1
- Bug: Wrong path for TS rendering · Line 212-212
-
internal/tschart/init.go - 1
- Incorrect build script · Line 337-337
-
cmd/nelm/chart_init.go - 1
- Input Validation Missing · Line 40-40
Additional Suggestions - 5
-
internal/tschart/runtime.go - 1
-
Invalid goja API usage · Line 104-104The ToObject(nil) call on line 104 violates goja's API, as Value.ToObject requires a valid *goja.Runtime. This could cause a panic during error formatting when JavaScript exceptions occur. If the runtime isn't available here, consider restructuring to pass it from the caller.
-
-
internal/tschart/transformer_unit_test.go - 2
-
Incomplete test assertions · Line 383-401The tests for generateVendorEntrypoint check the var declaration, require statements, and global assignment, but miss verifying the exports assignment. This could allow regressions if the exports line is removed from the function. Consider adding checks for 'exports.__NELM_VENDOR__ = __NELM_VENDOR__' in both test cases.
-
Test Isolation Improvement · Line 40-40The test for non-existent path uses a relative path not isolated in the test's tempDir, unlike other tests that use tempDir. This could potentially cause the test to fail if the current working directory has a file or directory with that name. Using filepath.Join(tempDir, "non-existent") would ensure proper isolation.
-
-
internal/tschart/init_test.go - 1
-
Missing Error Test Case · Line 155-257The test suite for InitChartStructure is missing coverage for the error case where the ts/ directory already exists. While the implementation correctly checks and returns an error in this scenario (matching InitTSBoilerplate's behavior), adding this test would ensure the error path is verified, preventing potential regressions if the check is accidentally removed.
-
-
pkg/action/chart_init.go - 1
-
Unused struct field · Line 17-17The TempDirPath field in ChartInitOptions is defined but not referenced in ChartInit or its callees. If this is for future temp dir logic, consider adding a comment; otherwise, remove to avoid confusion.
-
Review Details
-
Files reviewed - 21 · Commit Range:
9a6e649..37f16e7- cmd/nelm/chart.go
- cmd/nelm/chart_init.go
- cmd/nelm/chart_pack.go
- go.mod
- go.sum
- internal/chart/chart_render.go
- internal/tschart/console.go
- internal/tschart/engine.go
- internal/tschart/engine_unit_test.go
- internal/tschart/init.go
- internal/tschart/init_test.go
- internal/tschart/runtime.go
- internal/tschart/test_helpers_test.go
- internal/tschart/transformer.go
- internal/tschart/transformer_dir.go
- internal/tschart/transformer_mem.go
- internal/tschart/transformer_unit_test.go
- internal/tschart/tschart_test.go
- pkg/action/chart_init.go
- pkg/featgate/feat.go
- ts/nelm-types/src/index.ts
-
Files skipped - 2
- ts/nelm-types/package.json - Reason: Filter setting
- ts/nelm-types/tsconfig.json - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Golangci-lint (Linter) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
AI Code Review powered by
| Icon: string; | ||
| APIVersion: string; | ||
| Condition: string; | ||
| Tags: string; |
There was a problem hiding this comment.
The Tags field is incorrectly typed as string, but Helm Chart.yaml specifies tags as a list of strings, so it should be string[] to match the expected data structure.
Code Review Run #aee90b
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| } | ||
|
|
||
| if featgate.FeatGateTypescript.Enabled() { | ||
| jsRenderedTemplates, err := renderJSTemplates(ctx, originalChartPath, chart, renderedValues) |
There was a problem hiding this comment.
For remote charts, originalChartPath is the remote URL, but TypeScript rendering requires the local filesystem path where the chart is downloaded. Use chartPath instead, which is updated by downloadChart to point to the local directory.
Code suggestion
Check the AI-generated fix before applying
--- a/internal/chart/chart_render.go
+++ b/internal/chart/chart_render.go
@@ -212,1 +212,1 @@
- jsRenderedTemplates, err := renderJSTemplates(ctx, originalChartPath, chart, renderedValues)
+ jsRenderedTemplates, err := renderJSTemplates(ctx, chartPath, chart, renderedValues)
Code Review Run #aee90b
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| "description": "TypeScript chart for %s", | ||
| "main": "src/index.ts", | ||
| "scripts": { | ||
| "build": "npx tsc --noEmit", |
There was a problem hiding this comment.
The build script only type-checks without emitting compiled files, which doesn't match standard build expectations and ignores the tsconfig.json outDir setting. This could confuse users expecting a working build.
Code Review Run #aee90b
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
This comment was marked as off-topic.
This comment was marked as off-topic.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
2 participants
This PR is based on #497 and #500
Closes #54
I plan to provide more detailed examples in https://github.com/DmitryBochkarev/nelm-ts-examples
This is quite a big PR. I can try to split it into separate commits. 1000 lines of 2000 all implementation code are TS project initialization code(
nelm chart create), and 2/3 of the rest of the code is tests - so 1,000 lines of actual feature implementation.This PR is slightly diverged from original proposal regarding target js bundle file name. I named it
ts/chart_render_main.jsso it is explicit and hard to randomly create such file.TypeScript types a more simple than propose in #500
but it can be fixed later.
What is implemented
What is not implemented
What I need help with