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

Adjust tests for 6da99130 #1

Closed
wants to merge 2 commits into from
Closed

Adjust tests for 6da99130 #1

wants to merge 2 commits into from

Conversation

JCB-K
Copy link

@JCB-K JCB-K commented Jun 14, 2021

Hi! 👋 . I noticed that the tests on this PR import-js#2121 were passing with and without the bug fix, so I adjusted them so that they pick up the regression correctly.

The particular bug only shows up when:

  • you import a submodule from a ES package
  • the module exists on disk in node_modules
  • it's not listed in package.json

The test previously was checking for a package that was listed in package.json, so that meant it passed regardless of the bug being fixed or not.

I also made a small adjustment to the code, I'll comment it below.

if(!realPackageName){
throw new Error(`cant find real package name for import ${name}`);
}

Copy link
Author

Choose a reason for hiding this comment

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

instead of defaulting to an empty package name (which is what line 133 effectively did), throw an error when the package name is not found. This will make it easier to find bugs like this in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant