Skip to content

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

bodinsamuel
Copy link
Collaborator

@bodinsamuel bodinsamuel commented May 30, 2025

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
  • ???
  • Profit

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 legacy nango.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 from nango.yaml to pure TypeScript (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. • UpdatesCLI`` 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 flows

Potential 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 description

Codegen/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

@bodinsamuel bodinsamuel self-assigned this May 30, 2025
Copy link

linear bot commented May 30, 2025

@@ -0,0 +1,1146 @@
import { spawn } from 'node:child_process';
Copy link
Collaborator Author

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

@bodinsamuel bodinsamuel marked this pull request as ready for review June 2, 2025 08:49
@bodinsamuel bodinsamuel requested a review from a team June 2, 2025 08:50
Copy link
Collaborator

@TBonnin TBonnin left a 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 only id. 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 than id 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

@bodinsamuel
Copy link
Collaborator Author

nango.batchDelete used to accept a object with only id. 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 than id as optional?

Indeed, I'll change that thanks

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)

I think this one is acceptable, but if you can send me the code just to be sure 👍🏻

@TBonnin
Copy link
Collaborator

TBonnin commented Jun 2, 2025

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:

export async function onWebhookPayloadReceived(
    nango: NangoSync,
    payload: any,
): Promise<void> {
    await nango.log('Webhook payload received', payload);

    const contact = {
        id: SOME_RANDOM_ID,
        created_at: new Date(),
        updated_at: new Date(),
        first_name: 'first',
        last_name: 'last',
        email: 'first.last@email.com',
        active: true,
    };
    await nango.batchSave([contact], 'HubspotContact');
}

with the model in nango.yaml.

    HubspotContact:
        id: string
        created_at: string
        updated_at: string
        first_name: string
        last_name: string
        email: string

@kaposke
Copy link
Contributor

kaposke commented Jun 2, 2025

Just FYI I'm reviewing and testing as well

Copy link
Contributor

@kaposke kaposke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might not be with migrations but with zero-yaml itself:
z.date() in models.ts becomes date (invalid lowercase) in .nango/schema.ts, which then fails creation of the json schema.
image
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants