-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
esm: fix misleading error when import empty package.json #49728
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
1addfa1
to
8946387
Compare
8946387
to
57dc1e2
Compare
The problem isn't really that |
The question is how to determine if a |
2fc8c0f
to
902b0b7
Compare
Updated |
4ee33aa
to
eb6d5cb
Compare
@himself65 are you still interested in continueing this? There are outstanding comments, and failing tests. |
Oops! Sorry... closed the wrong PR tab 😅 |
Let me fix this |
eb6d5cb
to
f23c1e4
Compare
working on it |
fixed! |
fd4bc58
to
1a50565
Compare
idk why format-cpp action will delete half of the file. any idea? @redyetidev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #49728 +/- ##
==========================================
- Coverage 88.52% 88.52% -0.01%
==========================================
Files 660 660
Lines 190900 190900
Branches 36631 36638 +7
==========================================
- Hits 168998 168997 -1
+ Misses 15097 15086 -11
- Partials 6805 6817 +12
|
Rebase with the latest changes to |
3155734
to
77abfd1
Compare
There are some relevant test failures, PTAL |
() => legacyMainResolve(packageJsonUrl, { main: './index.node' }, packageJsonUrl), | ||
{ message: /index\.node/, code: 'ERR_MODULE_NOT_FOUND' }, | ||
{ message: /No package entry point defined for package/, code: 'ERR_MODULE_NOT_FOUND' }, |
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.
IIUC, there is an entry point defined, './index.node'
, but the resolution still fails because the file doesn't exist. This change would likely be confusing.
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.
yeah make sense, so I'm gonna check wheher user request a file or a module then give a differnt error
I might pick this issue again because I see this error message and confusing me for hours😅 |
ce27df1
to
d0b5261
Compare
c23e1ea
to
011628e
Compare
Fixes: #49674