Skip to content
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

feat: TS types for updates with readonly array values #829

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

duncanbeevers
Copy link
Contributor

🐞 Permit readonly arrays

Related issue: #828

Test case demonstrating TS type error.

image

@@ -282,7 +283,9 @@ describe('model', () => {
filter: { foo: 'foo' },
},
},
];
] as const;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also still use the bulkWrite() with an inline argument which is not marked as const? Do we have such a test? Let's add one, if not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a test that restores the original operations declaration, as well as one that tests an inline operations definition (and preserved the as const test, added here)

Comment on lines 228 to 242
$inc?: OnlyFieldsOfType<DeepReadonly<TSchema>, NumericType | undefined>;
$min?: PaprMatchKeysAndValues<DeepReadonly<TSchema>>;
$max?: PaprMatchKeysAndValues<DeepReadonly<TSchema>>;
$mul?: OnlyFieldsOfType<DeepReadonly<TSchema>, NumericType | undefined>;
$rename?: Record<string, string>;
$set?: PaprMatchKeysAndValues<TSchema>;
$setOnInsert?: PaprMatchKeysAndValues<TSchema>;
$unset?: OnlyFieldsOfType<TSchema, any, '' | 1 | true>;
$addToSet?: SetFields<TSchema>;
$pop?: OnlyFieldsOfType<TSchema, readonly any[], -1 | 1>;
$pull?: PullOperator<TSchema>;
$push?: PushOperator<TSchema>;
$pullAll?: PullAllOperator<TSchema>;
$set?: PaprMatchKeysAndValues<DeepReadonly<TSchema>>;
$setOnInsert?: PaprMatchKeysAndValues<DeepReadonly<TSchema>>;
$unset?: OnlyFieldsOfType<DeepReadonly<TSchema>, any, '' | 1 | true>;
$addToSet?: SetFields<DeepReadonly<TSchema>>;
$pop?: OnlyFieldsOfType<DeepReadonly<TSchema>, readonly any[], -1 | 1>;
$pull?: PullOperator<DeepReadonly<TSchema>>;
$push?: PushOperator<DeepReadonly<TSchema>>;
$pullAll?: PullAllOperator<DeepReadonly<TSchema>>;
$bit?: OnlyFieldsOfType<
TSchema,
DeepReadonly<TSchema>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

All this wrapping seems not DRY at all. Can we optimize this code somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see a simple way to do so, though I feel the duplication is pretty well concentrated, living only here where the major tributaries of the update methods feed into the PaprUpdateFilter defintion, and nowhere else.

* Break many expectations out to individual tests
@duncanbeevers
Copy link
Contributor Author

@avaly
We got a different set of failures on media-providers with these changes. However, I went ahead and linked my locally-built papr into media-providers and made one simple adjustment. This change does make this a breaking change, since PropertyType is part of the public contract.

diff --git a/@types/mongodb.d.ts b/@types/mongodb.d.ts
index c9004704d4..2ce4be1716 100644
--- a/@types/mongodb.d.ts
+++ b/@types/mongodb.d.ts
@@ -36,6 +36,6 @@ declare global {
   // This type is extracted from `papr.PaprMatchKeysAndValues`, because that type is too complex for our usage here,
   // and it would introduce more than 3 minutes on the `tsc` check times.
   export type MongoAttributes<TSchema> = {
-    [Property in mongodb.Join<papr.NestedPaths<TSchema, []>, '.'>]?: papr.PropertyType<TSchema, Property>;
+    [Property in mongodb.Join<papr.NestedPaths<TSchema, []>, '.'>]?: Mutable<papr.PropertyType<TSchema, Property>>;
   };
 }

@duncanbeevers duncanbeevers requested a review from avaly June 17, 2024 11:28
Copy link
Collaborator

@avaly avaly left a comment

Choose a reason for hiding this comment

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

If this is breaking the public interface of the PropertyType, then this commit needs to be marked as having breaking changes.

Please add BREAKING CHANGE: ...details in the commit message.

@CarlosLozanoHealthCaters
Copy link
Collaborator

@avaly I've tested it in our internal repos and it is compiling without errors.

Should we adapt other methods like insert to support readonly array too?

@avaly
Copy link
Collaborator

avaly commented Jun 17, 2024

Thanks for testing @CarlosLozanoHealthCaters! Yeah, we plan to extend support to all methods that accept user input.

@@ -193,7 +193,7 @@ export type PaprArrayElementsProperties<TSchema> = {
* }
*/
export type PaprArrayNestedProperties<TSchema> = {
[Property in `${KeysOfAType<PaprAllProperties<TSchema>, Record<string, any>[]>}.$${
[Property in `${KeysOfAType<PaprAllProperties<TSchema>, readonly Readonly<Record<string, any>>[]>}.$${
Copy link
Collaborator

@avaly avaly Jul 8, 2024

Choose a reason for hiding this comment

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

Why are there two readonlys here?

Copy link
Collaborator

@ejmartin504 ejmartin504 Jul 8, 2024

Choose a reason for hiding this comment

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

I had the same question but convinced myself it was correct -- I think Readonly makes the record readonly, and then the readonly makes the entire array readonly?

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.

4 participants