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

Rewrite module and _modulePrefix #31

Closed
tleunen opened this issue Aug 13, 2015 · 1 comment · Fixed by #32
Closed

Rewrite module and _modulePrefix #31

tleunen opened this issue Aug 13, 2015 · 1 comment · Fixed by #32

Comments

@tleunen
Copy link

tleunen commented Aug 13, 2015

I know this is a library initially for Facebook internal projects but instead of copying the script, I was trying to reuse what you did here and noticed something in the script rewrite-modules.

The default value for _modulePrefix is ./.

var modulePrefix = context.state.opts._modulePrefix || './';

I think the default value should be an empty string, because if you require an external module from your node_modules folder, you don't want to prefix it with ./.

For my needs, and because my project uses external modules, I would need the default to be an empty string, but because of the line above, you'll understand it's not possible.

If for your needs the default should stay ./, would at least be possible to check if the option is set?

@zpao
Copy link
Member

zpao commented Aug 13, 2015

That case is specifically to handle rewriting @providesModule requires, which we do assume end up all being relative, so we wouldn't change the default. I vaguely remember having a case where I wanted something to be unprefixed but then I just added the single case to the module map.

I had some confused comment trying to figure out what you were trying to do before I realized the actual problem - that '' is falsy so we're ignoring your default even though you specified it. Yea, that's a good catch and we should be checking for it being set.

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

Successfully merging a pull request may close this issue.

2 participants