Fixes: #56518 - Warning Message for .cjs Files Using import Statements #56618
Conversation
…y.cc - Added a utility function GetFileExtension to extract file extensions from filenames. - Enhanced the require_esm_warning logic to dynamically generate appropriate error messages based on file extensions. - Included a TODO comment to enhance error messages further with specific package.json file locations if applicable. - Cleaned up and removed redundant code for better maintainability and clarity.
- Extracted warning message logic into a dedicated `GetWarningMessage` function. - Encapsulated file extension handling and warning generation for improved reusability. - Updated relevant code sections to use the new factory-style function. - Aligns with existing design patterns and improves maintainability.
- Added new tests for invalid imports in .cjs files. - Added tests for loading .mjs files in CommonJS packages. - Refactored assertions to align with project lint rules. - Verified all tests pass with Node.js tools and addressed related errors. - Formatted src/node_contextify.cc using clang-format for consistent code style. Refs: nodejs#56518
- Updated `GetWarningMessage` in `node_contextify.cc` to handle `.cjs` files and emit appropriate warnings. - Normalized the warning message in tests to simplify comparison and improve readability. - Added specific tests to verify warnings for invalid imports in `.cjs` files and handling of `.mjs` in CommonJS packages. Note: The test currently compares the exact string value for warnings. This could be optimized by validating only the critical parts of the warning message. Refs: nodejs#56518
nodejs-github-bot
added
c++
|
|
||
| std::string GetWarningMessage(const std::string& file_extension) { | ||
| if (file_extension == ".cjs") { | ||
| return "Cannot use 'import' statement in a .cjs file. " |
There was a problem hiding this comment.
This could be also caused by e.g. top-level await. It would be confusing to directly suggest import as the cause of the parse error without checking the error messages (which are available in as cjs_message in CompileFunctionForCJSLoader).
| // Convert an int to a V8 Name (String or Symbol). | ||
| Local<Name> Uint32ToName(Local<Context> context, uint32_t index) { | ||
| return Uint32::New(context->GetIsolate(), index)->ToString(context) | ||
| return Uint32::New(context->GetIsolate(), index) |
There was a problem hiding this comment.
Can you revert the unrelated formatting changes? These would make backporting harder.
| std::string file_extension = GetFileExtension(filename_utf8.ToStringView()); | ||
| // TODO(wongprom): Enhance require_esm_warning to include | ||
| // the specific package.json file location, if applicable. | ||
| std::string require_esm_warning = GetWarningMessage(file_extension); |
There was a problem hiding this comment.
It would be better to only construct right before emitting the warning, otherwise it's going to be an overhead even on paths that would just retry with a successful detection and not emit the warning.
| `Error [ERR_REQUIRE_ESM]: require() of ES Module ${required} from ${requiringCjsAsEsm} not supported.\n` | ||
| ) | ||
| ); | ||
| assert.ok( |
There was a problem hiding this comment.
Can you either use assert.match or spawnSyncAndAssert (see https://github.com/nodejs/node/blob/99e0d0d2186065aa8ca9ab7cd94e1637bb166220/test/common/README.md#spawnsyncandexitcommand-args-spawnoptions-expectations )? Otherwise when mismatches happen it would be very difficult to see why without modifying the source and re-running it in e.g. the CI.
Summary
This PR introduces the following changes to address issue #56518:
Add a specific warning for
.cjsfiles usingimportstatements:.cjsfiles do not supportimportand advises them to userequire()instead.Update tests:
test/es-module/test-cjs-esm-warn.jsto validate the warning message.Refactored
src/node_contextify.cc:GetFileExtensionandGetWarningMessagehelper functions for better code readability and modularity.Details
Testing:
make -j6 test. All tests passed successfully.make lint.Changes:
jimmy.cjstest file after confirming its purpose was fulfilled during development.Developer's Certificate of Origin 1.1
By making a contribution to this project, I certify that:
Additional Notes