Conversation
Add validation to warn users when the "type" field in package.json contains an invalid value. Previously, values like "CommonJS" (wrong case) would silently fall back to typeless behavior. Now a clear warning message is displayed indicating the expected values are "commonjs" or "module". Fixes: nodejs#60085
ljharb
left a comment
There was a problem hiding this comment.
tests look good to me, i'll let someone else review the C++
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #60180 +/- ##
==========================================
+ Coverage 88.52% 88.56% +0.03%
==========================================
Files 703 704 +1
Lines 207825 208092 +267
Branches 40003 40012 +9
==========================================
+ Hits 183976 184294 +318
+ Misses 15862 15811 -51
Partials 7987 7987
🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| if (field_value != "commonjs" && field_value != "module") { | ||
| fprintf(stderr, |
There was a problem hiding this comment.
This should be using the usual machinery (process.emitWarning() in JS, or ProcessEmitWarning() in C++) to emit warning so that it can be suppressed with NODE_NO_WARNINGS=1 etc. or captured through event listeners by users.
There was a problem hiding this comment.
Done! Changed to use ProcessEmitWarning() instead of fprintf(). Let me know if the implementation looks correct.
| if (package_json == nullptr) { | ||
| return; | ||
| } | ||
| } |
|
|
||
| }); | ||
|
|
||
| describe('package.json type field validation', () => { |
There was a problem hiding this comment.
Can you add a new test file instead of appending it to the same test file? See
node/doc/contributing/writing-tests.md
Lines 30 to 32 in 3983ef6
There was a problem hiding this comment.
Done! I've added a new test file.
|
lint errors in some lines |
|
@ljharb, @mertcanaltin I have done the necessary changes, please have a look at my PR as per your convenience. |
|
This pull request has been marked as stale due to 90 days of inactivity. |
|
This pull request has been automatically closed after 30 days of inactivity following its stale status (no activity for a total of 120 days). |
|
i think this should land once somebody reviews the C++ |
|
Unfortunately you were left hanging without any feedback for quite a while. If you are interested in continuing with this PR, you'd need to:
|
Add validation to warn users when the "type" field in package.json contains an invalid value. Previously, values like "CommonJS" (wrong case) would silently fall back to typeless behavior. Now a clear warning message is displayed indicating the expected values are "commonjs" or "module".
Fixes: #60085