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.

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

module: add dynamic file-specific ESM warnings #56628

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

mertcanaltin
Copy link
Member

I improved the error message to include specific file and package.json details, trying to make it clearer and more actionable as requested in the TODO

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem. labels Jan 16, 2025
@mertcanaltin mertcanaltin force-pushed the mert/improve-esm-warning branch from 621136c to 777a249 Compare January 16, 2025 15:17
@mertcanaltin mertcanaltin added module Issues and PRs related to the module subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. labels Jan 16, 2025
@mertcanaltin mertcanaltin changed the title feat: add dynamic file-specific ESM warnings module: add dynamic file-specific ESM warnings Jan 16, 2025
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.04%. Comparing base (3fe8027) to head (2f8aeb6).
Report is 274 commits behind head on main.

Files with missing lines Patch % Lines
src/node_process_events.cc 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #56628      +/-   ##
==========================================
- Coverage   89.19%   89.04%   -0.16%     
==========================================
  Files         662      665       +3     
  Lines      191893   193420    +1527     
  Branches    36937    37281     +344     
==========================================
+ Hits       171164   172222    +1058     
- Misses      13573    13880     +307     
- Partials     7156     7318     +162     
Files with missing lines Coverage Δ
src/node_contextify.cc 80.74% <100.00%> (-0.63%) ⬇️
src/node_process_events.cc 59.09% <66.66%> (+0.07%) ⬆️

... and 158 files with indirect coverage changes

@mertcanaltin
Copy link
Member Author

mertcanaltin commented Jan 17, 2025

Could this be flakky? but tests to my local is success https://github.com/nodejs/node/actions/runs/12811945783/job/35722483892?pr=56628

Co-authored-by: Anna Henningsen <[email protected]>
@mertcanaltin
Copy link
Member Author

mertcanaltin commented Feb 12, 2025

The issue was that v8::String::NewFromUtf8 expected a const char*, but a std::string_view was passed. Converted using .data() and explicit length (cast to int). Special thanks to @addaleax for the help!

@mertcanaltin mertcanaltin added the review wanted PRs that need reviews. label Feb 13, 2025
mertcanaltin and others added 2 commits February 14, 2025 16:45
@legendecas legendecas added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Feb 17, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2025
@nodejs-github-bot
Copy link
Collaborator

@jasnell
Copy link
Member

jasnell commented Feb 18, 2025

The CI failures are relevant to this change. Looks like a test that needs fixing up and some lint issues to resolve.

@mertcanaltin
Copy link
Member Author

The CI failures are relevant to this change. Looks like a test that needs fixing up and some lint issues to resolve.

thanks, I updated

@mertcanaltin mertcanaltin added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 19, 2025
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Feb 19, 2025
Copy link
Contributor

Failed to start CI
   ⚠  Something was pushed to the Pull Request branch since the last approving review.
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/13406809365

@aduh95 aduh95 added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Feb 19, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 19, 2025
@nodejs-github-bot
Copy link
Collaborator

@mertcanaltin
Copy link
Member Author

lint seems to be warning about the use order but my local cpp-linter says everything is working fine

13:06:32 using statements aren't sorted in 'src/node_process_events.cc'.
13:06:32 	Line 17: Actual: v8::Nothing, Expected: v8::NewStringType
13:06:32 	Line 18: Actual: v8::Object, Expected: v8::Nothing
13:06:32 	Line 19: Actual: v8::String, Expected: v8::Object
13:06:32 	Line 20: Actual: v8::Value, Expected: v8::String
13:06:35 	Line 21: Actual: v8::NewStringType, Expected: v8::Value
13:06:35 make: *** [Makefile:1549: tools/.cpplintstamp] Error 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. esm Issues and PRs related to the ECMAScript Modules implementation. module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. review wanted PRs that need reviews. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants