-
Notifications
You must be signed in to change notification settings - Fork 491
feat(cli): migrate to zero #4138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,1146 @@ | |||
import { spawn } from 'node:child_process'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't except deep review here, it's the fruit of 2weeks of bug chasing and edge cases found in customers scripts. Half of the code is AI slop that I refined manually.
It's massive, tried to breakdown and add comments but it's a bit hard to digest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't reviewed the migration script but I tested it locally. I ran into 2 main breaking changes. There are not due to the migration itself but to the compiler being more strict:
nango.batchDelete
used to accept a object with onlyid
. Now the compiler expect a full object with all attributes. That might be an issue in scenarios where the full object to delete is not known. Should the type for batchDelete have all fields other thanid
as optional?- in a sync derived from the hubspot prebuilt sync with createdAt field set to type string. Passing a Date used to be allowed. Now the compiler emits an error (Date <> string)
Let me know if you want me to share the code
Indeed, I'll change that thanks
I think this one is acceptable, but if you can send me the code just to be sure 👍🏻 |
I agree it is acceptable and quite easy to fix. Here is the code if you want to check:
with the model in nango.yaml.
|
Just FYI I'm reviewing and testing as well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes
Fixes https://linear.app/nango/issue/NAN-3245/migration-command
nango migrate-to-zero-yaml
Add massive script to migrate full codebase to new syntax. On best case it migrates everything without a single error, in most cases there are a few types error at the end (e.g: import leftover, unstrict typings that becomes invalid).
Tested on a few customers with great success but it's probably missing a few edge cases.
Change export to import in index.ts
I realized there was some TS conflict because they all export the same things, it doesn't really matter since I'm not compiling this file but merely using it has declaration.
Try to use cli-test setup to fix windows test
Not sure if it's that but worth the shot, otherwise I'll create another PR for the fix
Tests
Caution
This operation is destructive, git commit or save your folder before testing
node packages/cli/dist/index.js migrate-to-zero-yaml
feat(cli): Add migration command for zero-yaml and update CLI migration strategy
This PR introduces a major migration capability to the Nango CLI, adding the
migrate-to-zero-yaml
command. It automates the migration from the legacynango.yaml
config-plus-scripts approach to a pure TypeScript, 'zero-yaml' structure. This includes a comprehensive script using 'jscodeshift' to refactor the codebase, handling script transformation (Syncs, Actions, OnEvents), model conversion to Zod schemas, auto-generating an index.ts for all scripts, and automating package.json and dependency updates. Extensive test coverage and snapshot testing are included. The PR also refactors several internals to support the new workflow, with new package dependencies and adjustments to CLI commands.Key Changes:
• Implements
migrate-to-zero-yaml
CLI
command to migrate integration codebases fromnango.yaml
to pureTypeScript
(zero-yaml
).• Adds a large migration script (
toZeroYaml`.ts`) utilizing jscodeshift to auto-transform Sync, Action, and `OnEvent` scripts, model imports, and refactor default exports. • Converts all legacy export patterns to side-effectful imports in the generated index.ts for easier
ESM/
CJScompatibility. • Generates models.ts with Zod schemas/types from parsed yaml/model definitions, with robust topological sorting for type dependencies. • Upgrades testing: adds extensive unit and snapshot tests for the migration logic. • Updates
CLI`` internals: new command registered, help text improved, dependency upgrades (notably adds jscodeshift, Zod), and relevant bugfixes (symlink handling, edge cases, package.json handling).• Refactors compilation, definition import/export, and migration toolchains for the new zero-yaml architecture.
• Handles legacy and edge-case imports/exports, batch method type arguments, and transformation pipeline with many fallbacks for failed cases.
Affected Areas:
• packages/cli/lib/migrations/
toZeroYaml
.ts (major new file)•
CLI
command registration and entrypoint (lib/index.ts)• Migration, model, and compilation internals
• Testing: migration unit tests and snapshots
• Dependency management (package.json, lockfile, Zod, jscodeshift etc.)
•
TypeScript
code transformation and codegen flowsPotential Impact:
Functionality: Enables a path for users to migrate to a new TypeScript-centric project structure, affecting all future integration development, and deprecates nango.yaml in favor of direct code. Destructive by design: removes nango.yaml, rewrites scripts, and may cause temporary type errors requiring manual fixes.
Performance: Batch file processing, code transformation, and npm install automation may increase CPU and I/O usage during migration. Performance is not impacted in runtime integrations.
Security: No new direct security surface, but users must be cautious when running destructive migration commands, especially with symlinks which are partly guarded.
Scalability: Handles large codebases with many scripts/models, using globbing and optimized dependency sorting. The logic should scale to projects with many files, but migration may take time on large codebases.
Review Focus:
• Correctness of the migration script transformations (model, sync, action,
onEvent
logic).• Edge case handling: imports, exports, legacy code patterns, symlinked files, type generation.
• Idempotence and error recovery: does the
PR
safely handle failed migrations?• Test coverage for migration logic (are the most critical code paths backed by good snapshot/unit tests?).
• Impact on existing users and compatibility (does this upgrade present migration blockers or breaking changes?).
Testing Needed
• Run
nango migrate-to-zero-yaml
on various real-world integration projects to verify migration accuracy.• Manually review migrated scripts for lingering type errors or incomplete refactoring (not all edge cases are covered).
• Check generated models.ts and index.ts for valid imports and typing correctness.
• Validate
CLI
commands and helpers after package.json/lockfile re-organization.• Run all new and existing unit/snapshot tests on multiple environments (including Windows, due to symlink handling logic).
Code Quality Assessment
packages/cli/lib/migrations/toZeroYaml.ts: Large, complex, well-commented; imperative in places but leverages best-available code transformation tools. Some AI-generated code refined manually, per author's note.
packages/cli/lib/index.ts: Command registration clear; adds migration command without impacting other CLI flows.
packages/cli/lib/zeroYaml/compile.ts: Small refinements for error handling and entrypoint logic; robust for new import/export patterns.
packages/cli/lib/migrations/toZeroYaml.unit.test.ts: Good test data and coverage: tests transformation and edge cases using Vitest and snapshotting.
packages/cli/lib/migrations/snapshots/toZeroYaml.unit.test.ts.snap: Exhaustive snapshot output validating migration results.
Best Practices
Migration:
• Automated transformation with dry-run and warning comments
• Automated backup recommendation in
PR
descriptionCodegen/Testing:
• Extensive snapshot tests
• Topological sort for type dependencies
Cli:
• Clearer
CLI
command registration• Comprehensive error and symlink handling
Possible Issues
• Not all code patterns and legacy import/export quirks may be covered-some manual refactoring could be required post-migration.
• Symlinks are skipped but not fully protected; could leave out required files if relied upon in the integration.
• Large code generation and AST transformation logic is inherently difficult to fully test-hidden logical bugs may surface in corner cases.
• Destructive operation-accidental overwrites cannot be rolled back; backups must be recommended.
• Complex package.json dependency migration could behave unexpectedly in some project setups.
This summary was automatically generated by @propel-code-bot