-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
module: add module.customConditions #57688
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.
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
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
9efaba7
to
6f23b07
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #57688 +/- ##
==========================================
+ Coverage 89.96% 90.07% +0.10%
==========================================
Files 640 640
Lines 188454 188489 +35
Branches 36892 36971 +79
==========================================
+ Hits 169546 169781 +235
+ Misses 11608 11422 -186
+ Partials 7300 7286 -14
🚀 New features to boost your workflow:
|
6f23b07
to
6bbb2e1
Compare
There was a problem hiding this 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
.
This should help with nodejs#57688 and others
I put up #57723 as a potential way to easily convert a SafeSet to a user-exposable Set, which might help this PR. |
This should help with nodejs#57688 and others
6bbb2e1
to
cae9e94
Compare
Thanks for the reviews! |
66cc00b
to
0789c21
Compare
Thanks, I've updated the code! |
There was a problem hiding this 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?
This comment was marked as outdated.
This comment was marked as outdated.
As far as I know, that’s the only way. |
The CI on Jenkins is failing, but I think the failure is not related to the change in this PR. |
0789c21
to
21d5f0c
Compare
I’ve rebased the PR in case that was a blocker. Let me know if there’s anything else I should do. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems sensible to me.
@JakobJingleheimer Assuming that the blocker of this PR is the CI on Jenkins, would you re-trigger the CI? |
This comment was marked as outdated.
This comment was marked as outdated.
cac31a9
to
0986bde
Compare
be5a488
to
05d07df
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think https://github.com/sapphi-red/node/blob/05d07dfb01ad34bfe1d236505b627d6f4465fd74/doc/api/cli.md?plain=1#L477-L502 will need to be updated.
I have never heard of this CLI option before, and neither the docs in this PR nor the docs I have linked make things clear to me.
05d07df
to
861529e
Compare
Add module.customConditions which exposes the custom resolution conditions specified by the user. Fixes: nodejs#55824
861529e
to
e87d6f5
Compare
Add
module.customConditions
which exposes the custom resolution conditions specified by the user.Fixes: #55824