optimize InferRawDocType #15588
optimize InferRawDocType #15588
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request optimizes the TypeScript types transitively referenced from InferRawDocType to improve type performance by an average of 50% without changing behavior. The optimization adds schema name discrimination for more efficient type comparisons and includes comprehensive benchmarking.
- Adds
schemaNameproperty to SchemaType instances for efficient schema discrimination - Introduces type-level benchmarks using
@ark/attestto measure type instantiation performance - Expands test coverage for
InferRawDocTypewith additional test cases for optionality, timestamps, and various definition types
Reviewed Changes
Copilot reviewed 5 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/types/inferrawdoctype.test.ts | Adds comprehensive test cases for InferRawDocType covering optionality, timestamps, and various schema definition types |
| test/schemaname.test.js | Tests the new schemaName property on both constructor and instance level |
| package.json | Adds @ark/attest dependency and new benchmark script for type performance testing |
| lib/schemaType.js | Adds schemaName property to SchemaType instances mirroring the constructor property |
| benchmarks/typescript/infer.bench.mts | Implements type-level benchmarks measuring InferRawDocType performance improvements |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
test/schemaname.test.js
Outdated
| const start = require('./common'); | ||
| const Schema = start.mongoose.Schema; | ||
|
|
||
| describe.only('schemaname', function() { |
There was a problem hiding this comment.
The test suite uses describe.only which will cause only this test to run, skipping all other tests in the codebase. This should be changed to describe to prevent accidentally disabling other tests.
| describe.only('schemaname', function() { | |
| describe('schemaname', function() { |
Copilot uses AI. Check for mistakes.
|
Thanks for the very detailed PR, this looks very cool! I need to try attest - my current TypeScript perf optimization workflow is quite painful. I'm fine with the runtime changes, and I'll look at the formatting changes some more - right now Mongoose doesn't have an automated formatter so your formatting changes are not wrong. However, it looks like Any idea what might be causing those? |
|
@vkarpov15 Whoops, sorry I missed that script! Most of these issues were just due to the new intersections being simplified to not include There was one tricky issue with the Let me know if you have any questions! |
|
@vkarpov15 Just updated this with the changes from 8.18. I've also tested it across Vanta's monorepo which uses defines hundreds of schemas using Mongoose and resolved the only error which was an infinite depth error inferring Let me know what a path would look like to getting this merged! |
|
Thanks! I'll re-review and target merging this for 8.19. Do you work at Vanta? |
| // can be efficiently checked like: | ||
| // `[T] extends [neverOrAny] ? T : ...` | ||
| // to avoid edge cases | ||
| type neverOrAny = ' ~neverOrAny~'; |
There was a problem hiding this comment.
I'd prefer to use Mongoose's IfAny<> type here if possible, this check seems a bit brittle, bit this is mostly a pedantic concern, not blocking.
There was a problem hiding this comment.
In addition to the check being slightly more performant, the main reason I'd recommend avoiding IfThen style utilities in place of conditional types is that all expressions passed to these utilities must be fully evaluated when the type is instantiated.
On the other hand, when you have a conditional type like this one, or even slightly less efficiently if you were to use something like IsAny<T> extends true ? ... : ..., only the matching branch would be evaluated.
This utility is based on the version I use throughout ArkType. It is definitely not fragile, but if you prefer something like IsAny that could also be a fine solution.
/** Can be used to test for the universal subtypes, `any` and `never`, e.g.:
*
* ```ts
* type isAnyOrNever<t> = [t] extends [anyOrNever] ? true : false
* ```
*
* The actual value is a string literal, but the only realistic subtypes
* of that literal are `any` and `never`.
*/
export type anyOrNever = " anyOrNever"
|
It is a little strange that this PR causes there to be more TypeScript instantiations and more TypeScript memory usage rather than less. Any ideas why that is? Stats from 8.18 release: https://github.com/Automattic/mongoose/actions/runs/17157925968/job/48679431153 Latest stats from this PR: https://github.com/Automattic/mongoose/actions/runs/17164018210/job/48824005687?pr=15588 |
The reason for this is likely just the new tests and benchmarks that were added as a part of this PR. Isolated benchmarks like those from
Awesome! I am currently doing some consulting work for Vanta to improve their type performance. |
|
I'll merge this into 8.19 branch for now so I can tinker with it a bit more, but overall I think this PR is excellent 👍 I'd just like to take a closer look as to why the top-line instantiations and memory usage are slightly higher. |
Summary
This pull request optimizes many of the types transitively referenced from
InferRawDocType.The goal is purely to improve type performance without changing the behavior of the types themselves.
It also adds some additional tests for
InferRawDocTypeand some type-level benchmarks that use@ark/attestto granularly measure type instantiations for a fewInferRawDocTypecalls, which were reduced by an average of 50% by this PR.Formatting
I wasn't sure how to handle formatting for the types in this repo, so I tried to create a prettier config that was similar to the formatting I found:
{ "printWidth": 120, "trailingComma": "none", "singleQuote": true, // helps flatten type-level ternaries, significantly increasing readability "experimentalTernaries": true }I only applied this formatting to
.d.tsfiles that I significantly changed.If there is a pre-existing tool I can use to format the types I added, I'd be happy to update my PR to use that.
Runtime Changes
There is only one new line of runtime logic, adding a
schemaNameproperty to SchemaType instances, mirroring the one that already exists on their constructors.This helps discriminate schema instances from one another so the types can be compared more efficiently without
IfEqualsand could be useful at runtime as well.If the team would prefer, I could also remove the runtime component and make this a private property so that it would still make schema types incompatible for more efficient comparisons without adding a new property visible to consumers at a type-level or at runtime.
Follow-ups
I notice many of the types e.g.
InferSchemaTypeclosely parallel the types I optimized inInferRawDocType. If the maintainers are interested in further such optimizations, similar strategies and benchmarking could likely be applied to many of the repo's types for even more significant improvements.