Skip to content

Conversation

@jedel1043
Copy link
Member

Implements the nonextensible-applies-to-private proposal.

Since it's such a small change and it's already on stage 3, I decided to not gate this behind the experimental feature.

@github-actions
Copy link

github-actions bot commented Oct 29, 2025

Test262 conformance changes

Test result main count PR count difference
Total 50,747 50,747 0
Passed 47,877 47,877 0
Ignored 2,060 2,060 0
Failed 810 810 0
Panics 0 0 0
Conformance 94.34% 94.34% 0.00%

@codecov
Copy link

codecov bot commented Oct 29, 2025

Codecov Report

❌ Patch coverage is 6.66667% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.10%. Comparing base (6ddc2b4) to head (cdcd089).
⚠️ Report is 593 commits behind head on main.

Files with missing lines Patch % Lines
core/engine/src/object/operations.rs 6.66% 14 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4511      +/-   ##
==========================================
+ Coverage   47.24%   57.10%   +9.85%     
==========================================
  Files         476      503      +27     
  Lines       46892    57464   +10572     
==========================================
+ Hits        22154    32812   +10658     
+ Misses      24738    24652      -86     

☔ 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 force-pushed the implement-nonextensible-applies-to-private branch from 4e040c4 to 2540fb2 Compare October 31, 2025 03:38
@jedel1043 jedel1043 force-pushed the implement-nonextensible-applies-to-private branch from 2540fb2 to e010f53 Compare November 14, 2025 01:35
@jedel1043 jedel1043 marked this pull request as ready for review November 14, 2025 01:36
@jedel1043 jedel1043 added enhancement New feature or request Internal Category for changelog labels Nov 14, 2025
@jedel1043 jedel1043 added this to the v1.0.0 milestone Nov 14, 2025
@jedel1043 jedel1043 requested a review from a team November 14, 2025 02:06
Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

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

Looks great! Just some minor nitpicks 😄

) -> JsResult<()> {
// 1. If the host is a web browser, then
// a. Perform ? HostEnsureCanAddPrivateElement(O).
if !self.extensible() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: It would be nice if we added the spec step to know where this came from. 😄

Suggested change
if !self.extensible() {
// 1. If O.[[Extensible]] is false, throw a TypeError exception.
//
// NOTE: From <https://tc39.es/proposal-nonextensible-applies-to-private/#sec-privatefieldadd>
if !self.extensible() {

(false, false)
};

if !self.extensible() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: Same here:

Suggested change
if !self.extensible() {
// 2. If O.[[Extensible]] is false, throw a TypeError exception.
//
// NOTE: From <https://tc39.es/proposal-nonextensible-applies-to-private/#sec-privatemethodoraccessoradd>
if !self.extensible() {

Comment on lines +947 to +955
return if getter || setter {
Err(js_error!(
TypeError: "cannot add private accessor to non-extensible class instance"
))
} else {
Err(js_error!(
TypeError: "cannot add private method to non-extensible class instance"
))
};
Copy link
Member

Choose a reason for hiding this comment

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

Praise: I really like the error message Improvement 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request Internal Category for changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants