Skip to content

Conversation

rrogerc
Copy link

@rrogerc rrogerc commented Oct 7, 2025

This Pull Request closes #4435, implementing the Upsert proposal for weak map

It changes the following:

  • Adds WeakMap.prototype.getOrInsert method.
  • Adds WeakMap.prototype.getOrInsertComputed method.
  • Enforces object-only keys and callable callbacks.
  • Includes new tests for both methods to ensure correct behavior and reentrancy.

- Enforce object keys; callable callback; reentrancy-safe insert/update.
- Register methods, update constructor metadata, and add tests.
Copy link

codecov bot commented Oct 7, 2025

Codecov Report

❌ Patch coverage is 93.61702% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.58%. Comparing base (6ddc2b4) to head (de424b1).
⚠️ Report is 548 commits behind head on main.

Files with missing lines Patch % Lines
core/engine/src/builtins/weak_map/mod.rs 93.61% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4459      +/-   ##
==========================================
+ Coverage   47.24%   51.58%   +4.33%     
==========================================
  Files         476      504      +28     
  Lines       46892    51416    +4524     
==========================================
+ Hits        22154    26522    +4368     
- Misses      24738    24894     +156     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jedel1043 jedel1043 added enhancement New feature or request builtins PRs and Issues related to builtins/intrinsics hacktoberfest-accepted PR accepted for Hacktoberfest labels Oct 8, 2025
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Everything looks nice! I just have a couple of suggestions to improve the code.

})?;

// 3. If CanBeHeldWeakly(key) is false, throw a TypeError exception.
// In Boa this maps to: key must be an Object.
Copy link
Member

Choose a reason for hiding this comment

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

Could you change this to a TODO comment instead? It is true that this currently maps to key must be an Object, but hopefully in the future we'll have a proper CanBeHeldWeakly to also accept Symbols as valid keys.

Comment on lines +348 to +354
let map = object
.as_ref()
.and_then(JsObject::downcast_ref::<NativeWeakMap>)
.ok_or_else(|| {
JsNativeError::typ()
.with_message("WeakMap.getOrInsertComputed: called with non-object value")
})?;
Copy link
Member

@jedel1043 jedel1043 Oct 8, 2025

Choose a reason for hiding this comment

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

You can avoid downcasting multiple times by using JsObject::downcast instead. This will return a "typed" object that you can borrow from by using the borrow and borrow_mut methods, and the type system will know that you're working with a JsObject that has a NativeWeakMap inside.

@jedel1043 jedel1043 added the waiting-on-author Waiting on PR changes from the author label Oct 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request hacktoberfest-accepted PR accepted for Hacktoberfest waiting-on-author Waiting on PR changes from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add getOrInsert and getOrInsertCallable to map & weak_map
4 participants