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.

Sign up for GitHub

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

Jump to bottom

fix(npm): improve error message on directory import in npm package #19538

Merged
merged 6 commits into from
Jul 14, 2023
Merged

fix(npm): improve error message on directory import in npm package #19538

merged 6 commits into from
Jul 14, 2023

Conversation

ElianCordoba
Copy link
Contributor

@ElianCordoba ElianCordoba commented Jun 16, 2023 •
edited by dsherret
Loading

Old error:

error: Unable to load /Users/eliancordoba/Library/Caches/deno/npm/registry.npmjs.org/lostjs/1.1.14/common imported from file:https:///Users/eliancordoba/repos/clones/deno/.vscode/tests/bad-error.ts

Caused by:
    Is a directory (os error 21)

New error:

error: Directory import /Users/eliancordoba/Library/Caches/deno/npm/registry.npmjs.org/lostjs/1.1.14/common is not supported resolving import from file:https:///Users/eliancordoba/repos/clones/deno/.vscode/tests/bad-error.ts

Caused by:
    Is a directory (os error 21)

I think we should remove the file protocol but I'll leave it up to you

cc @dsherret

Closes #19366

@CLAassistant
Copy link

CLAassistant commented Jun 16, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@dsherret dsherret changed the title (fix:npm): Improve error message on cjs import in esm in npm package fix:(npm): improve error message on directory import in npm package Jun 27, 2023
@aapoalas aapoalas changed the title fix:(npm): improve error message on directory import in npm package fix(npm): improve error message on directory import in npm package Jul 4, 2023
aapoalas
aapoalas previously requested changes Jul 4, 2023
View reviewed changes
Copy link
Collaborator

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me (especially with the TODO), but a test is failing: nonexistent_file test needs its expectations updated.

dsherret
dsherret reviewed Jul 14, 2023
View reviewed changes
cli/module_loader.rs Outdated Show resolved Hide resolved
dsherret
dsherret approved these changes Jul 14, 2023
View reviewed changes
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dsherret dsherret enabled auto-merge (squash) July 14, 2023 16:12
@dsherret dsherret merged commit 0223ad7 into denoland:main Jul 14, 2023
11 checks passed
bartlomieju pushed a commit that referenced this pull request Jul 19, 2023
@ElianCordoba @dsherret @bartlomieju
fix(npm): improve error message on directory import in npm package (#…
2bf2438 
…19538)

Co-authored-by: David Sherret <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsherret dsherret dsherret approved these changes

@aapoalas aapoalas aapoalas left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve directory import error
4 participants
@ElianCordoba @CLAassistant @dsherret @aapoalas