Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.79%. Comparing base (d2052ab) to head (5d062bc).
Report is 3 commits behind head on master.

@@           Coverage Diff            @@
##           master    #6120    +/-   ##
========================================
  Coverage   99.79%   99.79%            
========================================
  Files         264      264            
  Lines       18825    18870    +45     
  Branches     4685     4469   -216     
========================================
+ Hits        18786    18831    +45     
  Misses         39       39            

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

maybe we could revert those recent changes, since they were added because of how structuredClone works in combination with jest, and we no longer use structuredClone internally?

i would focus more on how this happened instead of supporting it on this low level helper. this feels like you are fixing it on wrong level.

@B4nan You were right! We should not fix this the way I did, or even not fix it at all.

I was initially trying to replicate the behavior of v6.3.10, but in the end I think current behavior is safer. I've added an integration test to document the use-case.

In v6.3.10, when updating a JSON property in MongoDB from a valid date to an invalid date, the date was simply erased and replaced by the Unix time 1970-01-01T00:00:00.000Z. This is IMO dangerous and unwanted. It is very easy to make such a mistake by instantiating a date with an undefined value in the constructor: new Date(undefined).

After more thought, I think it is best to throw an error and explain that this behavior is unsupported. There are no "invalid dates" in the BSON format, so I don't think we should handle that at all.

In the last commit I pushed on this PR, I changed the function to throw an exception when we encounter such a case. This does not "fix" anything but makes it explicit that there is an issue with the user code. Investigating this made me find an issue in the codebase I'm working on, so this is a good thing 🙏

maybe we could revert those recent changes, since they were added because of how structuredClone works in combination with jest, and we no longer use structuredClone internally?

Yes probably! Let's settle on the desired behavior for updates with invalid dates, then with all the tests we've added recently, it should be safe to try 👌

I don't mind explicit exceptions as you did. Perf wise, I would rather store the getTime() values and compare those instead of using toISOString and comparing strings.

Good point! done