Repo restructuring to remove circular dependencies

[o-alexandrov]

Related issues

• Generate an ESM bundle #175 (comment)
• Remove circular dependencies #226

RFC

• bases shouldn't import other modules
  • for example, src/types/base.ts should be reconsidered
• api reused in other modules should be modularized, for example:
  • src/parser.ts should be split into two files
    • src/parser/index.ts & src/parser/types.ts so the circular dependency between ZodError & parser is removed
      • and a similar approach could be done on other instances of circular dependency

[colinhacks]

I don't know how to do this without putting everything into one file. Circular dependencies are supposed to be allowed as long as you're careful about loading order. There's a reason base.ts imports all those things, it needs them to provide methods like .optional(). Perhaps something like this would solve the problem: https://medium.com/visual-development/how-to-fix-nasty-circular-dependency-issues-once-and-for-all-in-javascript-typescript-a04c987cf0de

[o-alexandrov]

Circular dependencies are supposed to be allowed as long as you're careful about loading order.

• I have to disagree. It is theoretically possible to add builds/bundlers' configs and add bundling of zod to tests. Then, we could start experimenting with the loading order, which is really non-trivial because many modules depend on each other. It'd be an odd way, but one that would guarantee the building/bundling w/ zod is safe.
  • whereas if we remove circular deps completely, we can forget about a possibility of having such issue & not to test it

I believe the two points in the RFC should completely remove all circular deps we have in the project.

• I'll attempt to create a PR with necessary restructuring to examine after [WIP] - attempt to use "internal" pattern to fix circular deps #231 is closed or if it hangs for more than 2 weeks.
• the only difficulty I predict to have is with classes, I will probably have to modularize some parts of interdependent classes

Personally, I don't believe it's possible to get rid of circular deps with the internal pattern.

• I created a [WIP] - attempt to use "internal" pattern to fix circular deps #231 PR though indicating the problem isn't resolved using such pattern
  • I like the ease of using madge to observe the issue is still present

[colinhacks]

Zod has been building/bundling and working fine for a long time with circular dependencies. The most recent issue with bundling is #222 and it was due to a dumb mistake on my part: using .constructor.name === 'ZodNever' to see if an object schema's catchall is a ZodNever instance, which breaks when the code is minimized. Fixed now.

The ESM stuff is a different beast and has other issues, but at the end of the day circular dependencies are possible in both module systems (commonjs or ESM). The hard part is structuring the code so both are happy. When I naively added ESM support last week, the code built fine but broke in runtime because the loading order stuff wasn't correct.

Madge is too strict since it doesn't allow any cycles. A better way to check for issues in the built version of Zod is to run the tests using the post-build version of Zod, instead of the .ts source. Doing that would have caught the ESM errors last week before they got published to NPM.

[o-alexandrov]

I believe I understand your position on circular dependencies:

• the primary concern on strict validation for circular dependencies, you think it'd negatively affect the desire of new contributors to make changes
  • why not keeping the strict validation as an internal tool to gradually rethink the design to get rid of them completely?
  • since zod is in beta, it's the perfect time to make restructuring changes, as breaking changes should be assumed to have a possibility of happening in beta

I have a different opinion, but you know better what's best for zod.
Just a couple of other arguments in favor of removal of circular dependencies completely, running against source files, besides these:

• last time I used zod as ESM (in env w/ terser & webpack 5), the result was a decrease of less than 2KB from the initial occupied space by zod of about 12KB gzipped.
  • even though I used a single model (because of individually bundled functions for FaaS) defined with zod (just .object, .parse, and some validation of a couple of primitives data types)
    • I believe the potential problems are:
      • circular dependencies
      • usage of interdependent classes, because this way you make the minificator unable to understand what parts of the class are used & unused because of the dynamic references

And as always, these are just what I, a mere individual, can think of right away.

[colinhacks]

I just don't think it's possible to remove circular dependencies without combining everything back into one file. Look at the imports in base.ts. You have a subclass (e.g. array.ts) importing the superclass and the superclass importing the subclass. Unless everything is in one file you can't get around that, as far as I know. Have you tried?

[o-alexandrov]

@colinhacks I have tried and resolved most of them before locally, but then I read the recommendation to follow the Medium article and decided to demonstrate first that article doesn't resolve the issue.
I plan to clean up the PR on circular dependencies later this week, since:

• it seems you would approve the changes, if they wouldn't hurt the readability (modularity) in the project

[mneumann]

FYI, I have been hit by the same issue today using zod together with rollup:

Circular dependencies
node_modules/zod/lib/cjs/types/base.js -> node_modules/zod/lib/cjs/parser.js -> node_modules/zod/lib/cjs/types/base.js
node_modules/zod/lib/cjs/index.js -> node_modules/zod/lib/cjs/types/string.js -> node_modules/zod/lib/cjs/types/base.js -> node_modules/zod/lib/cjs/parser.js -> node_modules/zod/lib/cjs/index.js
node_modules/zod/lib/cjs/types/base.js -> node_modules/zod/lib/cjs/parser.js -> /usr/home/mneumann/Collections/Dev/b790/b790-frontend/node_modules/zod/lib/cjs/types/base.js?commonjs-proxy -> node_modules/zod/lib/cjs/types/base.js
...and 5 more

And then at runtime in the browser:

Uncaught (in promise) DOMException: type 'module' in RegistrationOptions is not implemented yet.See https://crbug.com/824647 for details

Would be great to resolve this issue!

[sylvanaar]

Re: modularization and circular dependencies - one trick is to use dynamic imports as much as possible (ie. everywhere) - then refine the loading a bit based on use.

[colinhacks]

@o-alexandrov zod@3.0.0-alpha.7 now contains no circular dependencies.