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

Check importPath extension with 'dot' #672

Merged
merged 1 commit into from
Jan 4, 2018
Merged

Check importPath extension with 'dot' #672

merged 1 commit into from
Jan 4, 2018

Conversation

dplusic
Copy link
Contributor

@dplusic dplusic commented Nov 21, 2016

Assume that importPath is 'exceljs'.
The path does not have an extension.
But it ends with 'js' and will be handled as having an extension.
This misleads the module.
Fixed that by checking an extension with 'dot'.

@coveralls
Copy link

coveralls commented Nov 21, 2016

Coverage Status

Coverage remained the same at 94.86% when pulling c555ef1 on dplusic:bugfix/extensions into bfdc2bb on benmosher:master.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM, but would you mind adding a test case that would fail without your change?

@dplusic
Copy link
Contributor Author

dplusic commented Nov 21, 2016

I added the test case.

@coveralls
Copy link

coveralls commented Nov 21, 2016

Coverage Status

Coverage remained the same at 94.86% when pulling 1494cc2 on dplusic:bugfix/extensions into bfdc2bb on benmosher:master.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Hmm, why are there changes but no diffs in tests/files/internal-modules/package.json and tests/files/node_modules/exceljs/excel.js and tests/files/node_modules/exceljs/package.json?

@dplusic
Copy link
Contributor Author

dplusic commented Nov 21, 2016

tests/files/node_modules/exceljs/excel.js is added for resolving. To resolve importPath, there should be js file. tests/files/node_modules/exceljs/index.js is also possible, but I want to follow the structure of 'exceljs'. The content is empty because it is not executed as same as other test files(e.g. tests/files/node_modules/@org/package/index.js).

tests/files/node_modules/exceljs/package.json is added to reference excel.js. Without this file, resolver finds only index.js. This file is not empty, github just suppressed the diff. You can show the diff by clicking it.

tests/files/internal-modules/package.json is added to simulate Two package.json Structure. With this structure and problem of misjudging extension(the first problem of this issue, that is, without my change) together, resolver reports error message. This makes test fail. This file is empty because only the existence affects the problem. The content does not matter.

Copy link
Collaborator

@jfmengels jfmengels left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @dplusic!
@ljharb do you still want more changes for this?

@ljharb ljharb added the bug label Jan 4, 2018
@ljharb ljharb self-assigned this Jan 4, 2018
@coveralls
Copy link

coveralls commented Jan 4, 2018

Coverage Status

Coverage increased (+0.03%) to 96.212% when pulling 3a03aca on dplusic:bugfix/extensions into 523789f on benmosher:master.

@ljharb
Copy link
Member

ljharb commented Jan 4, 2018

Took care of the rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants