Skip to content
This repository has been archived by the owner on Oct 2, 2021. It is now read-only.

Revert a commit with data.loaders (fix #10) #11

Merged
merged 1 commit into from
Feb 1, 2015

Conversation

DenisIzmaylov
Copy link
Contributor

We should not support custom loaders for manifest main files

We should not support custom loaders for manifest main files
@lpiepiora
Copy link
Owner

@DenisIzmaylov thanks for your contribution, would it be possible to create a test case, which reproduces this issue, so we can avoid this situation in the future?

Or maybe you could tell me how can I reproduce it, so I can write a test case?

@mdreizin
Copy link

@lpiepiora This PR (#9) breaks some modules which defined in config under module.loaders section, for instance https://www.npmjs.com/package/json-loader.

module: {
        loaders: [{
            test: /\.json$/,
            loader: 'json-loader'
        }]
    }

and data.loaders.concat(bowerLoaderPath + "?" + bowerLoaderParams) should be reverted.

@lpiepiora
Copy link
Owner

@mdreizin do you mean this PR (that is DenisIzmaylov@4bd85f9) or the commit that this PR reverts?

@mdreizin
Copy link

@lpiepiora The following PR #9.

@lpiepiora
Copy link
Owner

@mdreizin so you're in favour of merging PR made by @DenisIzmaylov, correct ?

@mdreizin
Copy link

@lpiepiora we've looked deeper into this issue and found out that it's not likely to be an issue of this plugin and PR #9. For some reason, when json-loader is used together with bower-webpack-plugin, the latter retrieves it not directly, but via that loader, which causes an error for each bower package referenced. A temporary fix is to exclude bower.json from json-loader configuration, but a proper way to fix it would be to examine webpack core logic on that matter.

@lpiepiora
Copy link
Owner

@mdreizin I see, thanks for the information. Would you be able by a chance to provide self contained example of such a situation, so I can easily run it on my side?

@mdreizin
Copy link

@lpiepiora
Copy link
Owner

@mdreizin I looked at your project with the issue (sorry it took this long). I guess indeed this PR will fix it. The change the PR is recommending is data.loaders = [bowerLoaderPath + "?" + bowerLoaderParams], which is good, because the other PR changed this to data.loaders.concat(bowerLoaderPath + "?" + bowerLoaderParams), which I think was asking webpack to load the resource (bower.json file). This is not what we want, because when the file is resolved we want to use our own resolver to load it, as it actually parses it and loads the dependencies.

I'll merge this PR in seconds, and then release a fixed version. Let me know if you'll still need your workaround (tested with your project, and there it seems to be fine).

lpiepiora added a commit that referenced this pull request Feb 1, 2015
Revert a commit with data.loaders (fix #10)
@lpiepiora lpiepiora merged commit 7f46a8d into lpiepiora:master Feb 1, 2015
@mdreizin
Copy link

mdreizin commented Feb 2, 2015

@lpiepiora Now it works fine. Thanks a lot!

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

Successfully merging this pull request may close these issues.

3 participants