-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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.03% 88.04% +0.01%
==========================================
Files 652 652
Lines 183765 183764 -1
Branches 35858 35856 -2
==========================================
+ Hits 161771 161791 +20
+ Misses 15239 15227 -12
+ Partials 6755 6746 -9
|
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
Fixes: #49674