Skip to content

Fix fork() method crash in ESM environments due to directory imports #927

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

Closed
wants to merge 3 commits into from

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Aug 7, 2025

The Model.fork() method was crashing in ESM environments due to directory imports in the binary codec that don't work with Node.js ES module resolution. This affected users trying to use the library with modern ESM setups.

Problem

The issue occurred when calling model.fork() which internally uses Model.fromBinary() and Model.toBinary(). These methods depend on the binary codec (Encoder/Decoder) that used directory imports like:

import * as nodes from '../../../nodes';

Node.js ESM requires explicit file extensions and doesn't support directory imports without explicit index files, causing the following error:

Error [ERR_UNSUPPORTED_DIR_IMPORT]: Directory import '/path/to/nodes' is not supported resolving ES modules

Solution

  1. Fixed source code: Changed directory imports to explicit named imports in the core binary codec files:

    // Before
    import * as nodes from '../../../nodes';
    
    // After  
    import {
      ConNode,
      ValNode,
      RootNode,
      // ... other specific imports
    } from '../../../nodes';
  2. Enhanced build process: Added an automated ESM import fixer script that:

    • Adds .js extensions to relative imports
    • Converts directory imports to explicit index.js imports
    • Handles edge cases in the compiled ESM output
  3. Updated build script: The build:esm command now automatically applies ESM fixes post-compilation

Usage

The fix enables seamless ESM usage:

import { Model } from "json-joy/lib/json-crdt";

const model = Model.withServerClock();
model.api.set({ foo: "bar" });

// This now works correctly ✅
const forkedModel = model.fork();

// Binary serialization also works ✅
const binary = model.toBinary();
const restored = Model.fromBinary(binary);

Testing

  • All existing tests pass with no regressions
  • Fork functionality verified with both server and logical clocks
  • Binary serialization/deserialization confirmed working
  • Multiple fork scenarios tested successfully

Fixes #926.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copy link
Contributor Author

Copilot AI commented Aug 7, 2025

@streamich 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs.

I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review.

@Copilot Copilot AI changed the title [WIP] Forking a model crashes Fix fork() method crash in ESM environments due to directory imports Aug 7, 2025
@Copilot Copilot AI requested a review from streamich August 7, 2025 15:01
Copilot finished work on behalf of streamich August 7, 2025 15:01
@streamich streamich marked this pull request as ready for review August 8, 2025 17:33
@streamich streamich closed this Aug 8, 2025
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.

Forking a model crashes
2 participants