properties option - Githubissues

browserify / detective

Find all calls to require() no matter how deeply nested using a proper walk of the AST

Other

414 stars 60 forks source link

properties option #39

Closed charlierudolph closed 9 years ago

charlierudolph commented 9 years ago

A simpler interface for having support for both require and require.resolve instead of having to implement my own isRequire function.

charlierudolph commented 9 years ago

@substack any thoughts?

charlierudolph commented 9 years ago

@substack bump. I'm fine if you don't want to support this, just looking for some feedback

zertosh commented 9 years ago

Hey @charlierudolph, a few things:

Couldn't you do this by passing in a custom opts.isRequire? Especially since the return object's strings doesn't contain any info about where the string came from.
Since the most common use case is to not pass opts.properties, why not optimize for it by changing else if (callee.type === 'MemberExpression' && to else if (properties && callee.type === 'MemberExpression' &&? (That also means changing var properties = opts.properties).
Your test would also pass if you did require[resolve]('b');
For substack PRs, he likes the extra spaces.

charlierudolph commented 9 years ago

Couldn't you do this by passing in a custom opts.isRequire? Especially since the return object's strings doesn't contain any info about where the string came from.

Yeah that's how I currently support this. Just wanted to move some of that logic in here as I felt require.resolve is a pretty normal thing to do

Since the most common use case is to not pass opts.properties, why not optimize for it by changing else if (callee.type === 'MemberExpression' && to else if (properties && callee.type === 'MemberExpression' &&? (That also means changing var properties = opts.properties).

Your test would also pass if you did requireresolve;

I'm fine with that, are you suggesting adding a test case for that?

For substack PRs, he likes the extra spaces.

So he wants the trailing whitespace? My editor removes that automatically but I can try to put it back

zertosh commented 9 years ago

Yeah that's how I currently support this. Just wanted to move some of that logic in here as I felt require.resolve is a pretty normal thing to do

That's the point of providing a configurable test function, to keep thing custom checks out of the core project :smile: So if it's already working for you, then it makes sense to keep it out.