Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

module: add module.customConditions #57688

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

module: add module.customConditions #57688

wants to merge 1 commit into from
+61 −0

Conversation

sapphi-red
Copy link
Contributor

Add module.customConditions which exposes the custom resolution conditions specified by the user.

Fixes: #55824

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. labels Mar 30, 2025
@sapphi-red sapphi-red force-pushed the module/custom-conditions branch 2 times, most recently from 9efaba7 to 6f23b07 Compare March 30, 2025 08:55
@codecov Codecov
Copy link

codecov bot commented Mar 30, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.24%. Comparing base (8456a12) to head (0789c21).
Report is 5 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #57688      +/-   ##
==========================================
+ Coverage   90.23%   90.24%   +0.01%     
==========================================
  Files         630      630              
  Lines      185245   185261      +16     
  Branches    36301    36304       +3     
==========================================
+ Hits       167154   167192      +38     
- Misses      11006    11012       +6     
+ Partials     7085     7057      -28
Files with missing lines Coverage Δ
lib/module.js 100.00% <100.00%> (ø)

... and 31 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.

@sapphi-red sapphi-red force-pushed the module/custom-conditions branch from 6f23b07 to 6bbb2e1 Compare April 1, 2025 11:18
@ljharb ljharb added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 1, 2025
JakobJingleheimer
JakobJingleheimer reviewed Apr 1, 2025
View reviewed changes
Copy link
Member

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

I think in order to address the primordial issue, you need to use SafeSet within getUserConditions, and then within the getter, wrap the output of getUserConditions in Set.

@ljharb ljharb mentioned this pull request Apr 1, 2025
Closed
ljharb added a commit to ljharb/node that referenced this pull request Apr 1, 2025
@ljharb
lib: add SafeSet.prototype.toUnsafeSet
92a68f2 
This should help with nodejs#57688 and others
@ljharb
Copy link
Member

ljharb commented Apr 1, 2025

I put up #57723 as a potential way to easily convert a SafeSet to a user-exposable Set, which might help this PR.

ljharb added a commit to ljharb/node that referenced this pull request Apr 1, 2025
@ljharb
lib: add SafeSet.prototype.toUnsafeSet
fe02c30 
This should help with nodejs#57688 and others
@sapphi-red sapphi-red force-pushed the module/custom-conditions branch from 6bbb2e1 to cae9e94 Compare April 3, 2025 14:19
@sapphi-red
Copy link
Contributor Author

Thanks for the reviews!

sapphi-red
sapphi-red commented Apr 3, 2025
View reviewed changes
aduh95
aduh95 reviewed Apr 3, 2025
View reviewed changes
aduh95
aduh95 reviewed Apr 3, 2025
View reviewed changes
@sapphi-red sapphi-red force-pushed the module/custom-conditions branch 2 times, most recently from 1bb3c15 to 66cc00b Compare April 6, 2025 12:21
@sapphi-red
module: add module.customConditions
Loading
Loading status checks…
0789c21 
Add module.customConditions which exposes the
custom resolution conditions specified by the
user.

Fixes: nodejs#55824
@sapphi-red sapphi-red force-pushed the module/custom-conditions branch from 66cc00b to 0789c21 Compare April 6, 2025 12:27
@sapphi-red
Copy link
Contributor Author

Thanks, I've updated the code!

ljharb
ljharb approved these changes Apr 7, 2025
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

How would i polyfill this in a node that doesn’t have it? Would i have to trawl through NODE_OPTIONS and process.argv and the config file, or is there an easier way?

@JakobJingleheimer JakobJingleheimer added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 7, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 7, 2025
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/66099/

@sapphi-red
Copy link
Contributor Author

How would i polyfill this in a node that doesn’t have it? Would i have to trawl through NODE_OPTIONS and process.argv and the config file, or is there an easier way?

As far as I know, that’s the only way.

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

@Trott Trott

@targos targos

@aduh95 aduh95

@ljharb ljharb

@JakobJingleheimer JakobJingleheimer

Assignees
No one assigned
Labels
module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a way to get the enabled conditions
7 participants
@sapphi-red @nodejs-github-bot @ljharb @Trott @targos @JakobJingleheimer @aduh95