Remove circular dependencies

[o-alexandrov]

It lets us not to guess where the circular dependencies are.
Instead, we could have a tool that reports it to us (pls see a screenshot below).

I saw your comment and wanted to help:

It seems to be related to some tricky circular dependency issues that are doing to take some time to figure out, so I'm removing the ESM module in zod@2.0.0-beta.21.

[lo1tuma]

FWIW: I like this a lot and inspired by this PR I also tried to integrate madge in one of my own projects. But apart from detected circular dependencies it lacks a lot of other useful dependency linting rules. So I discovered dependency-cruiser which seems to be more advanced than madge.

[o-alexandrov]

@lo1tuma thank you, didn't know about dependency-cruiser
Would you mind pointing to a configuration you have set up in your projects so I could learn from you?

• it'd help me to contribute to it, if possible

@colinhacks could you please share your view on whether you support this direction?

[lo1tuma]

@o-alexandrov This is the config I came up with:

'use strict';

module.exports = {
    forbidden: [
        {
            name: 'no-circular',
            severity: 'error',
            from: {},
            to: {
                circular: true
            }
        },
        {
            name: 'no-orphans',
            severity: 'error',
            from: {
                orphan: true,
                pathNot: [
                    '\\.d\\.ts$',
                    'stryker.conf.js',
                    'prettier.config.js',
                    'dependency-cruiser.config.js',
                    'webpack.config.js',
                    '\\.test\\.(js|ts|jsx|tsx)$'
                ]
            },
            to: {}
        },
        {
            name: 'no-non-package-json',
            severity: 'error',
            from: {},
            to: {
                dependencyTypes: ['npm-no-pkg', 'npm-unknown']
            }
        },
        {
            name: 'not-to-unresolvable',
            severity: 'error',
            from: {},
            to: {
                couldNotResolve: true
            }
        },
        {
            name: 'no-duplicate-dep-types',
            severity: 'error',
            from: {},
            to: {
                dependencyTypes: ['npm'],
                moreThanOneDependencyType: true
            }
        },
        {
            name: 'not-to-spec',
            severity: 'error',
            from: {},
            to: {
                path: '\\.test\\.(js|ts|jsx|tsx)$'
            }
        },
        {
            name: 'not-to-dev-dep',
            severity: 'error',
            from: {
                path: 'src/',
                pathNot: ['\\.test\\.(js|ts|jsx|tsx)$']
            },
            to: {
                dependencyTypes: ['npm-dev'],
                moreThanOneDependencyType: false,
                pathNot: ['node_modules/@types/', '.d.ts$']
            }
        }
    ],
    options: {
        doNotFollow: {
            path: 'node_modules|build/',
            dependencyTypes: ['npm', 'npm-dev', 'npm-optional', 'npm-peer', 'npm-bundled', 'npm-no-pkg']
        },
        exclude: {
            path: 'build/'
        },
        moduleSystems: ['cjs', 'es6', 'tsd'],
        tsPreCompilationDeps: true,
        tsConfig: {
            fileName: 'tsconfig.json'
        },
        preserveSymlinks: false,
        combinedDependencies: true,
        reporterOptions: {
            dot: {
                collapsePattern: 'node_modules/[^/]+'
            }
        }
    }
};

I’m running this as an npm run-script via "lint:dependencies": "depcruise --config dependency-cruiser.config.js .".

[o-alexandrov]

@colinhacks
I plan to work on this issue later this week, by:

• cleaning this PR up, as we've made changes to linting & formatting
• replacing madge with dependency-cruiser
• manually fixing all circular dependencies trying to make as little changes as possible
  • I expect to make:
    • little importing adjustments
    • some restructuring of core logic into smaller modules that don't depend on anything
      • this should help with interdependent classes

[o-alexandrov]

Steps made:

1. merged my branch with Resolve rollup build issues #266

2. added dependency-cruiser

   • surprisingly, there were still lots of circular dependencies' instances (see screenshot below)

3. resolved most of the circular dependencies
   • @colinhacks it'd be time consuming for me to resolve the ones below, so it'd be great if you could take a look at the remaining issues below
     • in short, they all are due to ZodType <=> and some types that ZodType imports

[colinhacks]

This is great stuff!

I tried to warn you about those issues here: #266 (comment)

Subclasses of ZodType (e.g. ZodOptional) are imported into base.ts and are used in method definitions. You can't get around that.

There's no way to get rid of them without removing those methods, which I'm not willing to do. My plan is to fix this issues by controlling the loading order. Like you mentioned, this is a LOT easier now that there are only a few cycles to worry about.

Can you explain how to reproduce #215 in detail? I'm trying to bundle Zod with webpack and run some test scripts but I can't reproduce. I'm not comfortable adding the ES module back into the npm package until I've confirmed that those issues are resolved.

[colinhacks]

All the tests are failing for me. Were they working on your end?

Test suite failed to run:
    TypeError: Object prototype may only be an Object or null: undefined
        at setPrototypeOf (<anonymous>)

EDIT: I got this working, see below.

[colinhacks]

Okay I got this working again. I'm essentially using a simple version of the internal modules approach. The trick was to import all the circular dependencies from src/index instead of from the individual src/types/*.ts files.

// BEFORE
import { ZodArray } from "../../array";
import { ZodNullable, ZodNullableType } from "../../nullable";
import { ZodOptional, ZodOptionalType } from "../../optional";
import { ZodTransformer } from "../../transformer";

// AFTER
import {
  ZodArray,
  ZodNullable,
  ZodNullableType,
  ZodOptional,
  ZodOptionalType,
  ZodTransformer,
} from "../../../index";

I also had to disable these two rules in eslintrc.json:

"simple-import-sort/imports": 0,
"simple-import-sort/exports": 0,

They were sorting all the imports by file name which was messing up the loading order every time I saved the file.

[colinhacks]

I'm not sure why these issues started appearing, but they appear to be the same ones that were happening in #215. So hopefully this solution will solve the ESM issues as well. I just pushed an updated branch that contains my fixes: https://github.com/colinhacks/zod/tree/o-alexandrov-v2.

The next step is to test this with Webpack, Node, and Rollup to see if it works.

[o-alexandrov]

Thank you for resolving remaining issues.
I apologize, I cannot work on it in the next two weeks probably, because of some personal deadlines.

If anyone kindly could test this branch with their bundling tools, please do.
All you need to do is:

1. clone this official repo
2. checkout o-alexandrov-v2 branch & run yarn to install deps
3. build the project by running yarn build
4. link zod dependency in your project with the locally built project

// package.json
"dependencies": {
  "zod": "link:../path-to-cloned-zod"
}

5. kindly report, if you have any issues

[rundis]

Looks promising! Worked with snowpack 2.18.4 (which uses rollup I gather for cjs modules).

[colinhacks]

@o-alexandrov I pushed an updated version to the "o-alexandrov-v2" branch of Zod. Play around with it and see if you discover any issues. I had to re-introduce several circular dependencies but it was necessary to get the tests to run correctly.