knockout debug and release both being pulled in

[kmalakoff]

When I run webpack, it looks like both versions of knockout are being pulled in:

○ → webpack
Hash: 7f03fbcef4cb6cbdcfde
Version: webpack 1.3.3-beta2
Time: 402ms
 Asset    Size  Chunks             Chunk Names
app.js  322423       0  [emitted]  main
   [0] ./app.js 30 {0} [built]
   [1] ./~/knockout/build/output/knockout-latest.js 54210 {0} [built]
   [2] ./~/knockout/build/output ^\.\/.*$ 266 {0} [built]
   [3] ./~/knockout/build/output/knockout-latest.debug.js 260844 {0} [optional] [built]
   [4] (webpack)/buildin/module.js 241 {0} [built]

WARNING in ./~/knockout/build/output/knockout-latest.js
Critical dependencies:
7:222-229 require function is used in a way, in which dependencies cannot be statically extracted
 @ ./~/knockout/build/output/knockout-latest.js 7:222-229

WARNING in ./~/knockout/build/output/knockout-latest.debug.js
Critical dependencies:
22:24-31 require function is used in a way, in which dependencies cannot be statically extracted
 @ ./~/knockout/build/output/knockout-latest.debug.js 22:24-31

I'm not sure it this is an issue with webpack or knockout or some combination of them?

I have created a sample project here: https://github.com/kmalakoff/webpack_knockout_sample. Just run:

git clone git@github.com:kmalakoff/webpack_knockout_sample.git; cd webpack_knockout_sample; npm install; webpack

Thank you!

Kevin

[sokra]

knockout passes the require function to another function. This cannot be analysed statically.

http://webpack.github.io/docs/shimming-modules.html#broken-commonjs

require("imports?require=>false!knockout") should fix it.

This also means the require property of the knockout config doesn't work with a module id instead of a module name...

[jhnns]

knockout passes the require function to another function.

Where does that happen? Is it here? This should be resolved with a pull-request @kmalakoff, so other webpack/browserify users won't run into the same situation.

[sokra]

Yep. https://github.com/knockout/knockout/blob/master/build/fragments/amd-pre.js#L6

I think this is even wrong. The require is used here as AMD require. So it looks wrong to pass a CommonJS require.

@mbest has added it in knockout/knockout@847e1f1

[jhnns]

@kmalakoff Could you do a pull-request?

[kmalakoff]

Thank you for looking into this.

@sokra: if I understand correctly, you are saying that the static analysis warning is actually the cause of the library being included twice (eg. the issues are linked)? or is it just another unrelated issue?

@jhnns I think you are asking me to create a pull-request on Knockout with a patch for the way they are implementing their module system support? I'm really not an expert in this area (for example, I've never used AMD on a project) which is why I'm using webpack...leave it to experts like yourselves!

Of course, I understand that you guys also only have limited time and cannot get into issues on every incompatible library.

I could study one of my webpack builds, try to copy the pattern into knockout, and submit a pull request. If I understand correctly, there is a problem with this and this. If I create a patch, would one of you be willing to review it before I submit it as a pull-request?

@sokra: unfortunately, the workaround does not work well for libraries (like Knockback) to use webpack-style requires like require("imports?require=>false!knockout") because we run our code on client and server so require statements need to be Node.js compatible.

As an aside, we are considering webpack to replace Brunch as our client application build system (I ran into this knockout problem during my evaluations). We are still trying to find a workaround for compile-time resolution of require statements for our views. I've opened an issue on it.

[sokra]

see http://webpack.github.io/docs/using-loaders.html#usage

Instead of using the inline syntax require("imports?require=>false!knockout") you can (and should) use the config syntax:

{ test: /knockout.build.output.knockout-latest\.js/, loader: "imports?require=>false" }

require("knockout");

[kmalakoff]

@sokra awesome that there is another way to use the custom syntax. It seems to have worked to have the knockout to be included once. Thank you!

@jhnns would you be willing to review a pull-request to fix knockout?

[jhnns]

Sure, go ahead 😄

[sokra]

knockout/knockout@847e1f1#commitcomment-7555943

[mbest]

I think this is even wrong. The require is used here as AMD require. So it looks wrong to pass a CommonJS require.

I'm not so familiar with the difference between the two. Is it that the former accepts a callback function and the latter doesn't? If that's the case, does that mean that those using Knockout as a CommonJS module won't be able to use the "require" feature for modules?

[mbest]

Knockout currently prefers the CommonJS module system over AMD. If that preference were switched, would that help? It seems that the workaround proposed above would make webpack include Knockout as an AMD module.

[sokra]

CommonJs: require("string") returns module
AMD: require(["array", "of", "string"], function(called, with, modules) {...}

So yes Knockout as CommonJS module won't be able to use the "require" feature.

But as webpack relies on static analysis it won't be able to use the "require" feature at all... (even with AMD).

In general it's a bad idea to pass around require strings or the require function with webpack, as that is not statically analysable, except for some simple cases. You can do this with require.js, but the require.js optimizer will need help to include the correct modules.

[sokra]

http://knockoutjs.com/documentation/component-registration.html

The documentation tells that you need use an AMD module with the require feature...

[mbest]

Thanks for the details. I found that there's already a documentation section about using CommonJs-style require for components: http://knockoutjs.com/documentation/component-loaders.html#note-integrating-with-browserify

[kmalakoff]

@mbest would you be open to changing the module loading patterns and not having require being passed in for Knockout?

Is there an example of 'possiblyGetConfigFromAmd'? It seems like the only place where the require is used inside Knockout and maybe there is a different way to do it? Maybe the caller should be responsible for the async loading of config and just pass it to Knockout? (I'm just guessing here because I'm not sure what it is for)

[kmalakoff]

I've been playing around more with this.

When I use a bower install of knockout (from my own repo at https://github.com/kmalakoff/knockout) with the require statement, here's the output:

WARNING in ./bower_components/knockout/knockout.js
Critical dependencies:
22:24-31 require function is used in a way, in which dependencies cannot be statically extracted
 @ ./bower_components/knockout/knockout.js 22:24-31

WARNING in ./bower_components/knockout/bower.json
Module parse failed: /Users/kevin/Dev/OpenSource/Client/webpack-test-app/bower_components/knockout/bower.json Line 2: Unexpected token :
You may need an appropriate loader to handle this file type.
| {
|   "name": "knockout",
|   "version": "3.2.0",
|   "main": "knockout.js",
 @ ./bower_components/knockout ^\.\/.*$

WARNING in ./bower_components/knockout/.bower.json
Module parse failed: /Users/kevin/Dev/OpenSource/Client/webpack-test-app/bower_components/knockout/.bower.json Line 2: Unexpected token :
You may need an appropriate loader to handle this file type.
| {
|   "name": "knockout",
|   "version": "3.2.0",
|   "main": "knockout.js",
 @ ./bower_components/knockout ^\.\/.*$

But if I comment out the require statement (https://github.com/kmalakoff/knockout/tree/no_amd), everything just works.

[jhnns]

It looks like the problem is caused by possiblyGetConfigFromAmd. Could you remove it and run it again?

The error is because webpack includes everything in this folder (because the module's dependencies couldn't be analyzed statically) - and tries to load a json-file which requires the json-loader.

[kmalakoff]

Yeah, that's what the https://github.com/kmalakoff/knockout/tree/no_amd branch does - no errors in that case, but I'm not comfortable in removing core Knockout functionality.

I'm opening an issue on knockout that will argue that their API should change that I know what their component framework does.

I wonder why it is trying to load a json file. Is that something that can be disabled through webpack config so it succeeds?

[jhnns]

You could try the noParse-option. In tells webpack to not parse the modules that match the given RegExps. And since knockout don't really needs require(), it should be no problem.

But it's good to fix these things in the popular libraries we use. People should stop messing around with the require()-function. Dynamic dependencies are not necessary in most situations.

[iongion]

Sorry guys, but how is this solved in the end ?

[kmalakoff]

I ended up with a config file like:

module.exports =
  module:
    loaders: [
      {test: /knockout\/build\/output\/knockout-latest\.debug\.js/, loader: 'imports?require=>__webpack_require__'}
    ]
    noParse: [
      /knockout\/build\/output\/knockout-latest\.debug\.js/
    ]

[hirokiky]

I got a same problem and avoided by using kmalakoff's config #424 (comment)
thanks a lot.

[devel-pa]

I am trying for a month to go from Brunch to WebPack in my current project and I am out of ideas on how to make it work. There is a forum/place/mailing list, something that can describe how to do, for the beginning, the same thing in WebPack as in Brunch? My stack is Backbone/Marionette.

[kmalakoff]

@devel-pa unforuntately, webpack is not as simple to configure as Brunch because it does more like static analysis and it depends on the actual JavaScript libraries you use (and how well they support module systems).

I would image that maybe a Wiki page for library-by-library webpack settings recommendations would meet your needs since it depends on what you are using.

[jhnns]

Yep. But I think it's always the best to convince module author to provide CommonJS-compliant modules

[gaearon]

This looks similar: localForage/localForage#344.
Ideas?

[joshacheson]

#424 (comment) is not working for us when running webpack -p. Any ideas?

[bebraw]

@kmalakoff Any luck with the current version?

[kmalakoff]

@bebraw sorry, I haven't tried the latest version yet.

[bebraw]

@kmalakoff Want to have another look? I think I'll close this. If you can repro the issue, please reopen.