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

if _options.basedir exists, use it in external, exclude and ignore #1551

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tufandevrim
Copy link

Not sure if it was intentional or not, main option basedir (this._options.basedir) is being ignored in external, exclude and ignore functions. It is being taken into account in require function by the way https://github.com/substack/node-browserify/blob/master/index.js#L114

@jmm
Copy link
Collaborator

jmm commented May 5, 2016

Hello @tufandevrim, thanks for contributing. At a glance this does seem to make sense, but it'd help to have failing tests that show the current implementation doing the wrong thing.

(Side note: totally not your fault, but I noticed that these all have an undocumented opts param and read at least opts.basedir from it...ugh.)

@tufandevrim
Copy link
Author

tufandevrim commented May 10, 2016

Hi @jmm
Please take a look at the test I just added and let me know if it makes sense or not.
If you run this test without the changes proposed in this PR, it will fail unless you add basedir option explicitly to this line https://github.com/substack/node-browserify/pull/1551/files#diff-f101fe74380a155175b7b9240f1b1fcaR15

@jmm
Copy link
Collaborator

jmm commented May 18, 2016

Thanks @tufandevrim! I or someone will take a look when there's a chance.

@pushred
Copy link

pushred commented Jan 28, 2017

This threw me off today. Because of the undocumented opts param on ignore I assumed that this._options was the one to use.. I'd submit a PR for a doc update but seems this is the better approach if require already does this.

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

Successfully merging this pull request may close these issues.

None yet

3 participants