[mongodb] UpdateQuery is not readonly #44105

avaly · 2020-04-22T10:06:29Z

Please fill in this template.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Add or edit tests to reflect the change. (Run with npm test.)
Follow the advice from the readme.
Avoid common mistakes.
Run npm run lint package-name (or tsc if no tslint.json is present).

If changing an existing definition:

Include tests for your changes

There's no reason why an update object can't be mutated before being used. In complex applications building such an object can be influenced by many conditions and factors.

typescript-bot · 2020-04-22T10:22:37Z

@avaly Thank you for submitting this PR!

🔔 @CaselIT @alanmarcell @Dante-101 @mcortesi @EnricoPicci @AJCStriker @julien-c @daprahamian @Denys-Bushulyak @bastienar @sindbach @geraldinelemeur @various89 @angela-1 @lirbank @hector7 @floric @erikc5000 @Manc @jloveridge @Ranguna @HosseinAgha @albertossilva @peterblazejewicz @LinusU @taxilian @xamgore - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

typescript-bot · 2020-04-22T10:22:38Z

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

LinusU · 2020-04-22T10:30:37Z

Hmmm, this is an interesting one, the values that are being accepted by the functions are really the readonly variant, since the function accepting the values won't modify them.

If we remove readonly from types that are being accepted, it means that you can no longer pass readonly types into those functions (you can pass both normal and readonly things to a readonly accepting function, but you cannot pass readonly things to a normal accepting function).

I'm not really sure what the best approach here is, since the example that you added in the test is something that someone might want to do...

avaly · 2020-04-22T11:22:04Z

the values that are being accepted by the functions are really the readonly variant

Maybe I'm missing this obvious thing, but is this something that's defined somewhere (the fact that the functions are accepting only readonly parameters)?

CaselIT · 2020-04-22T11:32:04Z

Maybe I'm missing this obvious thing, but is this something that's defined somewhere (the fact that the functions are accepting only readonly parameters)?

I think that what he meant was that a function that accept a readonly variant, accepts also the not readonly one:

function foo(arg: readonly number[]) {}
foo([1,2])
foo([2,3] as const)

If you remove the readonly in the definition, the second call will error

avaly · 2020-04-22T11:51:36Z

I understood that part @CaselIT, but I am confused because all the functions that use UpdateQuery are not marking it as readonly, e.g.:

    updateOne(
        filter: FilterQuery<TSchema>,
        update: UpdateQuery<TSchema> | Partial<TSchema>,
        callback: MongoCallback<UpdateWriteOpResult>,
    ): void;

typescript-bot · 2020-04-22T13:18:48Z

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

Comparison details 📊

	master	#44105	diff
Batch compilation
Memory usage (MiB)	79.7	78.9	-1.0%
Type count	15618	15623	0%
Assignability cache size	5505	5515	0%

Language service
Samples taken	1867	1902	+2%
Identifiers in tests	1867	1902	+2%
`getCompletionsAtPosition`
Mean duration (ms)	345.6	347.0	+0.4%
Mean CV	8.8%	8.7%
Worst duration (ms)	451.4	474.0	+5.0%
Worst identifier	options	toArray
`getQuickInfoAtPosition`
Mean duration (ms)	348.2	349.3	+0.3%
Mean CV	9.4%	9.3%	-0.9%
Worst duration (ms)	443.4	475.6	+7.3%
Worst identifier	connect	connectionString

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

LinusU · 2020-04-23T10:47:42Z

One problem would be that the following would no longer work:

async function addTagsToPost (postId: string, tags: readonly string[]) {
  await posts.update({ _id: postId }, { tags: { $push: tags } })
 }

on the other hand, it fixes the problem that you stated so there is clearly pros to both approaches ☺️

I would say that I'm leaning towards making this change 👍, but it would be nice to see if any other maintainers wants to chime in ☺️

elibarzilay · 2020-04-24T22:42:08Z

Is this ready to merge?

typescript-bot · 2020-04-26T17:29:15Z

@avaly Unfortunately, this pull request currently has a merge conflict 😥. Please update your PR branch to be up-to-date with respect to master. Have a nice day!

andrewbranch · 2020-04-27T18:02:27Z

Is anyone able to determine how likely this is to cause breaks for people using these types today? I’m a little concerned about issues like the scenario @LinusU identified. It seems to me that it isn’t strictly necessary to propagate readonly into the library typings just to be able to build up a query object by mutation: playground. It’s nice to have a convenient contextual type, but if it causes pain for lots of users, it’s probably not worth it.

avaly · 2020-04-28T08:08:18Z

@andrewbranch Thanks for the feedback! I guess we can live with that workaround.

I'll close this PR.

typescript-bot · 2020-04-28T08:08:20Z

@avaly Thank you for submitting this PR!

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

🔔 @CaselIT @alanmarcell @Dante-101 @mcortesi @EnricoPicci @AJCStriker @julien-c @daprahamian @Denys-Bushulyak @bastienar @sindbach @geraldinelemeur @various89 @angela-1 @lirbank @hector7 @floric @erikc5000 @Manc @jloveridge @Ranguna @HosseinAgha @albertossilva @peterblazejewicz @LinusU @taxilian @xamgore - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
❌ Most recent commit is approved by type definition owners or DT maintainers

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "pr_number": 44105,
  "author": "avaly",
  "owners": [
    "CaselIT",
    "alanmarcell",
    "dante-101",
    "mcortesi",
    "EnricoPicci",
    "AJCStriker",
    "julien-c",
    "daprahamian",
    "denys-bushulyak",
    "BastienAr",
    "sindbach",
    "geraldinelemeur",
    "various89",
    "angela-1",
    "lirbank",
    "hector7",
    "floric",
    "erikc5000",
    "Manc",
    "jloveridge",
    "ranguna",
    "HosseinAgha",
    "albertossilva",
    "peterblazejewicz",
    "LinusU",
    "taxilian",
    "xamgore",
    "avaly"
  ],
  "dangerLevel": "ScopedAndTested",
  "headCommitAbbrOid": "77ac4d3",
  "headCommitOid": "77ac4d3554fe492d2ac23bf993d218ad4cd58873",
  "mergeIsRequested": false,
  "stalenessInDays": 0,
  "lastCommitDate": "2020-04-27T09:11:38.000Z",
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/44105/files",
  "hasMergeConflict": false,
  "authorIsOwner": true,
  "isFirstContribution": false,
  "popularityLevel": "Popular",
  "anyPackageIsNew": false,
  "packages": [
    "mongodb"
  ],
  "files": [
    {
      "filePath": "types/mongodb/index.d.ts",
      "kind": "definition",
      "package": "mongodb"
    },
    {
      "filePath": "types/mongodb/test/collection/updateX.ts",
      "kind": "test",
      "package": "mongodb"
    }
  ],
  "otherApprovalCount": 0,
  "ownerApprovalCount": 0,
  "maintainerApprovalCount": 0,
  "hasDismissedReview": false,
  "travisResult": "pass",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 0,
  "isChangesRequested": false
}

typescript-bot · 2020-04-28T08:08:21Z

@avaly Thank you for submitting this PR!

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

🔔 @CaselIT @alanmarcell @Dante-101 @mcortesi @EnricoPicci @AJCStriker @julien-c @daprahamian @Denys-Bushulyak @bastienar @sindbach @geraldinelemeur @various89 @angela-1 @lirbank @hector7 @floric @erikc5000 @Manc @jloveridge @Ranguna @HosseinAgha @albertossilva @peterblazejewicz @LinusU @taxilian @xamgore - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
❌ Most recent commit is approved by type definition owners or DT maintainers

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "pr_number": 44105,
  "author": "avaly",
  "owners": [
    "CaselIT",
    "alanmarcell",
    "dante-101",
    "mcortesi",
    "EnricoPicci",
    "AJCStriker",
    "julien-c",
    "daprahamian",
    "denys-bushulyak",
    "BastienAr",
    "sindbach",
    "geraldinelemeur",
    "various89",
    "angela-1",
    "lirbank",
    "hector7",
    "floric",
    "erikc5000",
    "Manc",
    "jloveridge",
    "ranguna",
    "HosseinAgha",
    "albertossilva",
    "peterblazejewicz",
    "LinusU",
    "taxilian",
    "xamgore",
    "avaly"
  ],
  "dangerLevel": "ScopedAndTested",
  "headCommitAbbrOid": "77ac4d3",
  "headCommitOid": "77ac4d3554fe492d2ac23bf993d218ad4cd58873",
  "mergeIsRequested": false,
  "stalenessInDays": 0,
  "lastCommitDate": "2020-04-27T09:11:38.000Z",
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/44105/files",
  "hasMergeConflict": false,
  "authorIsOwner": true,
  "isFirstContribution": false,
  "popularityLevel": "Popular",
  "anyPackageIsNew": false,
  "packages": [
    "mongodb"
  ],
  "files": [
    {
      "filePath": "types/mongodb/index.d.ts",
      "kind": "definition",
      "package": "mongodb"
    },
    {
      "filePath": "types/mongodb/test/collection/updateX.ts",
      "kind": "test",
      "package": "mongodb"
    }
  ],
  "otherApprovalCount": 0,
  "ownerApprovalCount": 0,
  "maintainerApprovalCount": 0,
  "hasDismissedReview": false,
  "travisResult": "pass",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 0,
  "isChangesRequested": false
}

bopfer · 2020-04-30T03:19:06Z

Is there an elegant workaround to the readonly's ? I have many Mongoose hooks that alter $set and/or $unset values. The recent changes to UpdateQuery have started enforcing the readonly, which broke things. Right now I am @ts-ignoreing the relevant lines since the code itself works fine.

typescript-bot added this to Check and Merge in Pull Request Status Board Apr 22, 2020

typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback Author is Owner The author of this PR is a listed owner of the package. labels Apr 22, 2020

avaly force-pushed the mongodb/updatequery-not-readonly branch from 89d7f93 to a202695 Compare April 22, 2020 11:25

typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label Apr 22, 2020

typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. and removed Awaiting reviewer feedback labels Apr 23, 2020

typescript-bot moved this from Check and Merge to Needs Author Attention in Pull Request Status Board Apr 26, 2020

typescript-bot added Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. and removed Merge:Express labels Apr 26, 2020

[mongodb] UpdateQuery is not readonly

77ac4d3

avaly force-pushed the mongodb/updatequery-not-readonly branch from a202695 to 77ac4d3 Compare April 27, 2020 09:11

typescript-bot moved this from Needs Author Attention to Check and Merge in Pull Request Status Board Apr 27, 2020

typescript-bot added Merge:Express and removed Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. labels Apr 27, 2020

avaly closed this Apr 28, 2020

Pull Request Status Board automation moved this from Check and Merge to Done Apr 28, 2020

typescript-bot added Where is Travis? and removed Owner Approved A listed owner of this package signed off on the pull request. labels Apr 28, 2020

typescript-bot added this to Waiting for Code Reviews in New Pull Request Status Board Apr 28, 2020

orta mentioned this pull request Apr 28, 2020

Don't do work on closed PRs DefinitelyTyped/dt-mergebot#16

Closed

orta removed this from Waiting for Code Reviews in New Pull Request Status Board May 5, 2020

elibarzilay removed this from Done in Pull Request Status Board Jul 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mongodb] UpdateQuery is not readonly #44105

[mongodb] UpdateQuery is not readonly #44105

avaly commented Apr 22, 2020

typescript-bot commented Apr 22, 2020 •

edited

typescript-bot commented Apr 22, 2020 •

edited

LinusU commented Apr 22, 2020

avaly commented Apr 22, 2020

CaselIT commented Apr 22, 2020 •

edited

avaly commented Apr 22, 2020

typescript-bot commented Apr 22, 2020

LinusU commented Apr 23, 2020

elibarzilay commented Apr 24, 2020

typescript-bot commented Apr 26, 2020

andrewbranch commented Apr 27, 2020

avaly commented Apr 28, 2020

typescript-bot commented Apr 28, 2020

typescript-bot commented Apr 28, 2020

bopfer commented Apr 30, 2020

[mongodb] UpdateQuery is not readonly #44105

[mongodb] UpdateQuery is not readonly #44105

Conversation

avaly commented Apr 22, 2020

typescript-bot commented Apr 22, 2020 • edited

typescript-bot commented Apr 22, 2020 • edited

LinusU commented Apr 22, 2020

avaly commented Apr 22, 2020

CaselIT commented Apr 22, 2020 • edited

avaly commented Apr 22, 2020

typescript-bot commented Apr 22, 2020

LinusU commented Apr 23, 2020

elibarzilay commented Apr 24, 2020

typescript-bot commented Apr 26, 2020

andrewbranch commented Apr 27, 2020

avaly commented Apr 28, 2020

typescript-bot commented Apr 28, 2020

Code Reviews

Status

typescript-bot commented Apr 28, 2020

Code Reviews

Status

bopfer commented Apr 30, 2020

typescript-bot commented Apr 22, 2020 •

edited

typescript-bot commented Apr 22, 2020 •

edited

CaselIT commented Apr 22, 2020 •

edited