Skip to content

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented Apr 1, 2025

This should help with #57688 and others.

This should help with nodejs#57688 and others
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Apr 1, 2025
Copy link

codecov bot commented Apr 2, 2025

Codecov Report

Attention: Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.23%. Comparing base (1c2d98d) to head (fe02c30).
Report is 27 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/per_context/primordials.js 33.33% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #57723   +/-   ##
=======================================
  Coverage   90.22%   90.23%           
=======================================
  Files         630      630           
  Lines      185073   185079    +6     
  Branches    36222    36222           
=======================================
+ Hits       166990   166998    +8     
+ Misses      11044    11040    -4     
- Partials     7039     7041    +2     
Files with missing lines Coverage Δ
lib/internal/per_context/primordials.js 98.66% <33.33%> (-0.53%) ⬇️

... and 22 files with indirect coverage changes

🚀 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.

Comment on lines +425 to +427
const set = new Set();
this.forEach((value) => primordials.SetPrototypeAdd(set, value));
return set;
Copy link
Contributor

@aduh95 aduh95 Apr 2, 2025

Choose a reason for hiding this comment

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

Can we write tests that validates no user-code would be run? I think this can be simplified to

Suggested change
const set = new Set();
this.forEach((value) => primordials.SetPrototypeAdd(set, value));
return set;
return new Set(this);

(given the triviality of the implementation, I wonder if it actually makes sense to add a method for that)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure! I couldn't find anywhere SafeSet is already tested, so i'm not sure where to add them - any suggestions?

I guess this is a valid simplification since it has a safe [Symbol.iterator] on it, in which case you're right, it's probably not needed. I was assuming add-at-construction-time wasn't an option.

@ljharb
Copy link
Member Author

ljharb commented Apr 3, 2025

Closing, given #57723 (comment)

@ljharb ljharb closed this Apr 3, 2025
@ljharb ljharb deleted the tounsafeset branch April 3, 2025 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants