-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
Throw on the main export and be silent when using resolveFrom.silent() #4
Conversation
index.js
Outdated
return null; | ||
if (silent) { | ||
try { | ||
_resolveFileName(fromDir, fromFile, moduleId); |
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.
This needs to return. Probably needs a test to confirm this is working.
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.
Funny, the tests were covering it and it worked, there was another return statement at the end of the function, for the "non silent" case.
because it didn't fall to the cache
it just worked :)
test.js
Outdated
t.is(m.silent('fixture', './nonexistent'), null); | ||
const resolveFromfixture = m.bind(null, 'fixture'); | ||
t.regex(resolveFromfixture('./fixture'), /fixture\/fixture\.js$/); | ||
const silentResolveFromfixture = m.silent.bind(null, 'fixture'); |
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.
The silent assertions should be in a separate test.
index.js
Outdated
@@ -2,7 +2,15 @@ | |||
const path = require('path'); | |||
const Module = require('module'); | |||
|
|||
module.exports = (fromDir, moduleId) => { | |||
const _resolveFileName = (fromDir, fromFile, moduleId) => { |
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.
Wondering why you prefixed this with underscores? Might have missed something in the conversation though.
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.
You're right, I removed the underscores.
As a response to this PR- #3
and - sindresorhus/import-from#1