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

Fixes #411 #413

Merged
merged 2 commits into from
Jul 14, 2016
Merged

Fixes #411 #413

merged 2 commits into from
Jul 14, 2016

Conversation

Satyam
Copy link
Contributor

@Satyam Satyam commented Jul 4, 2016

index.js contains the fix
webpack.config.js contains code that trips an error if fix not present.
Initial git clone test gave two errors which are not related to this fix.

Satyam added 2 commits July 4, 2016 15:40
Fix in index.js
Extra configuration on webpack.config.js to trip the error if not fied
Fix to previous PR 412
@benmosher
Copy link
Member

Thanks for this!

Looks good at first glance, I just need to check out the test failures and take a closer look at the Webpack spec.

@benmosher
Copy link
Member

Finally looked at the spec. No good docs on context that I could find, but this makes sense to me. Thanks!

@@ -14,7 +14,7 @@ module.exports = {
{ 'jquery': 'jQuery' },
'bootstrap',
function (context, request, callback) {
if (request === 'underscore') {
if (request === 'underscore' && context.indexOf('/') !== -1) {
Copy link
Member

Choose a reason for hiding this comment

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

@Satyam: can you explain why this extra predicate? seems to be unnecessary on OSX, and I think it might be why the tests fail on Windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is just to force an error when context is null since null has no indexOf method. Whatever produces an error when context is null would do just as well, just to make sure the fix is not reversed later on.

benmosher added a commit that referenced this pull request Jul 14, 2016
@benmosher benmosher mentioned this pull request Jul 14, 2016
@benmosher benmosher merged commit f39a05f into import-js:master Jul 14, 2016
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.

2 participants