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

"Critical dependencies" in Webpack #76

Closed
tiye opened this issue Mar 16, 2015 · 27 comments
Closed

"Critical dependencies" in Webpack #76

tiye opened this issue Mar 16, 2015 · 27 comments

Comments

@tiye
Copy link

tiye commented Mar 16, 2015

I got this warning when in Webpack:
webpack/webpack#196

[WDS] Warnings while compiling.
main.js:576 ./~/jsondiffpatch/src/main.js
Critical dependencies:
48:19-49 the request of a dependency is an expression
53:18-46 the request of a dependency is an expression
 @ ./~/jsondiffpatch/src/main.js 48:19-49 53:18-46
main.js:576 ./~/jsondiffpatch/src/formatters/index.js
Critical dependencies:
8:19-45 the request of a dependency is an expression
 @ ./~/jsondiffpatch/src/formatters/index.js 8:19-45

https://github.com/benjamine/jsondiffpatch/blob/master/src/main.js#L48

    var packageInfoModuleName = '../package.json';
    var packageInfo = require(packageInfoModuleName);
    exports.homepage = packageInfo.homepage;
    exports.version = packageInfo.version;

    var formatterModuleName = './formatters';
    var formatters = require(formatterModuleName);
    exports.formatters = formatters;

It's still working thank god. But is that ok we remove these line to get rid of the warnings?

@benjamine
Copy link
Owner

I'm not familiar with webpack, but those lines have a function, which is to expose part of the package.json info for consumers of the library.
I'm using a variable on that require so browserify doesn't try to bundle the package.json file into the browser bundle.
I'd be happy to fix that but only if there's a way to do it keeping that functionality (and still keep that file out of the browser bundle).

@tiye
Copy link
Author

tiye commented Mar 16, 2015

Webpack is like Browserify when packing modules in CommomJS(static analytics). I don't see a good solution here.
Maybe I should try that ContextReplacementPlugin mentioned in related issues instead.

@benjamine
Copy link
Owner

ok, I see, yes, reading that thread that seems to be their recommended solution.
Ideally you should be able to tell webpack to just ignore any require like that (as that code only runs on node.js, ie. when not bundled), browserify does that by default.

@sokra
Copy link

sokra commented Apr 25, 2015

There is a spec for ignoring modules for the browser: https://gist.github.com/defunctzombie/4339901#ignore-a-module

i. e.

"browser": {
  "./package.json": false,
  "./src/formatters": false
}

@aaronshaf
Copy link

This bit me too :(

@aaronshaf
Copy link

For now I am doing this:

import jsondiffpatchHtmlFormatter from 'jsondiffpatch/src/formatters/html'

@co-dh
Copy link

co-dh commented Oct 27, 2015

Can you keep the variable for package.json but remove the variable for ./formatters?
also exports formatters no matter it's browser or not.
like:

exports.formatters = require('./formatters');
if (environment.isBrowser) {
        exports.homepage = '{{package-homepage}}';
        exports.version = '{{package-version}}';
} else {
        var packageInfoModuleName = '../package.json';
        var packageInfo = require(packageInfoModuleName);
        exports.homepage = packageInfo.homepage;
        exports.version = packageInfo.version;

        // shortcut for console
        exports.console = formatters.console;
}

also do the same for texts.js

var dmp = require('../../public/external/diff_match_patch_uncompressed' );

so browserify can work?
I tried locally and it works fine.

Best Regards.

@cmelion
Copy link

cmelion commented Oct 28, 2015

I don't use browserify (using webpack), will the suggestion from @sokra work with browserify?

@co-dh
Copy link

co-dh commented Oct 28, 2015

I changed to webpack too because browserify don't support expression in require(expression).
I don't understand why formatters are not exported when not isBrowser.
It works for me with webpack like below:

let jsondiffpatch = require('jsondiffpatch')
let formatters = require('jsondiffpatch/src/main-formatters')

while jsondiffpatch are installed with npm.

@cmelion
Copy link

cmelion commented Nov 25, 2015

setting exprContextCritical: false in webpack.config worked for me.

http://webpack.github.io/docs/configuration.html#automatically-created-contexts-defaults-module-xxxcontextxxx

@oskarer
Copy link

oskarer commented Mar 2, 2016

Any updates on this? Is it safe to use @cmelion's solution?

@arbesfeld
Copy link

Hi, any recommendations for what to do here? I wasn't able to use @cmelion solution, and it would be nice to be able to include this without any major webpack configuration changes.

@cmelion
Copy link

cmelion commented Sep 15, 2016

@arbesfeld: Can you clarify what prevented you from using the webpack config hack?

    module: {
        exprContextCritical: false,
        preLoaders: [
            {
                test: /\.jsx?$/,
                exclude: ['node_modules', 'test'],
                loader: 'eslint-loader'
            }
        ],
        loaders: [ 
               .
               .
               .
        ]

@arbesfeld
Copy link

@cmelion I am getting this error when I add that:

Uncaught Error: Cannot find module '../package.json'.

@cmelion
Copy link

cmelion commented Sep 15, 2016

@arbesfeld can you get a stack trace?

@arbesfeld
Copy link

I'm having difficulty getting a stacktrace, but here is the source context of the thrown error. I added >>> to the line that is throwing.

***/ },
/* 80 */
/***/ function(module, exports, __webpack_require__) {

    var map = {
        "./contexts/context": 68,
        "./contexts/context.js": 68,
        "./contexts/diff": 67,
        "./contexts/diff.js": 67,
        "./contexts/patch": 70,
        "./contexts/patch.js": 70,
        "./contexts/reverse": 71,
        "./contexts/reverse.js": 71,
        "./date-reviver": 69,
        "./date-reviver.js": 69,
        "./diffpatcher": 64,
        "./diffpatcher.js": 64,
        "./environment": 63,
        "./environment.js": 63,
        "./filters/arrays": 74,
        "./filters/arrays.js": 74,
        "./filters/dates": 76,
        "./filters/dates.js": 76,
        "./filters/lcs": 75,
        "./filters/lcs.js": 75,
        "./filters/nested": 73,
        "./filters/nested.js": 73,
        "./filters/texts": 77,
        "./filters/texts.js": 77,
        "./filters/trivial": 72,
        "./filters/trivial.js": 72,
        "./formatters/annotated": 81,
        "./formatters/annotated.js": 81,
        "./formatters/base": 82,
        "./formatters/base.js": 82,
        "./formatters/console": 83,
        "./formatters/console.js": 83,
        "./formatters/html": 91,
        "./formatters/html.js": 91,
        "./formatters/index": 92,
        "./formatters/index.js": 92,
        "./formatters/jsonpatch": 93,
        "./formatters/jsonpatch.js": 93,
        "./main": 62,
        "./main-formatters": 95,
        "./main-formatters.js": 95,
        "./main-full": 96,
        "./main-full.js": 96,
        "./main.js": 62,
        "./pipe": 66,
        "./pipe.js": 66,
        "./processor": 65,
        "./processor.js": 65
    };
    function webpackContext(req) {
        return __webpack_require__(webpackContextResolve(req));
    };
    function webpackContextResolve(req) {
>>>     return map[req] || (function() { throw new Error("Cannot find module '" + req + "'.") }());
    };
    webpackContext.keys = function webpackContextKeys() {
        return Object.keys(map);
    };
    webpackContext.resolve = webpackContextResolve;
    module.exports = webpackContext;
    webpackContext.id = 80;

@cmelion
Copy link

cmelion commented Sep 15, 2016

@arbesfeld I was hoping to see where the reference to ../package.json was coming from.
You might try searching your src folder for '../package.json' just to confirm you don't have an invalid import/require.

@arbesfeld
Copy link

I don't have any ../package.json in my code, I'm assuming it's coming from here: https://github.com/benjamine/jsondiffpatch/blob/master/public/build/jsondiffpatch-full.js#L3401

When I copy and pasted the library and commented out these lines everything worked: https://github.com/benjamine/jsondiffpatch/blob/master/public/build/jsondiffpatch-full.js#L3397-L3411

@cmelion
Copy link

cmelion commented Sep 15, 2016

Prior to using NPM to import the dependency I believe I ended up hacking a copy of the lib as well.
How are you importing json diffpatch?
We currently use it in the following manner:
package.json:

    "jsondiffpatch": "^0.1.37",

Store:

import jsondiffpatch from 'jsondiffpatch';


var Store = Reflux.createStore({

    // see http://benjamine.github.io/jsondiffpatch/demo/index.html
    // objectHash function is used only when objects are not equal by ref
    diffpatcher: jsondiffpatch.create({
        arrays: {
            detectMove: false
        },
        objectHash: function(obj, index) {
            return obj.id || obj.offeringId || '$$index:' + index;
        }
    }),

   .
   .
   .

    saveScheduleChanges() {
        const diff = {
             .
             .
             .
            delta: this.diffpatcher.diff(this.state.originalTimeslices, this.state.timeslices),
            .
            .
            .
        };

        // Add IDs to identify the slice and/or offering
        this.decorateDelta(diff.delta);
.
.
.
.

    },

@gabidobo
Copy link

@arbesfeld You're right, those requires(exp) generate dynamic contexts, which resolve to webpack bundling every file in ./ - and, of course, neither '../package.json' nor './formatters' are in that list. Setting exprContextCritical to false just gets rid of the build-time warning.

For me, what did the trick was using a ContextReplacementPlugin, explicitly mapping all dynamic requires to actual files:

plugins = [
  new webpack.ContextReplacementPlugin(/.*/, path.resolve(__dirname, 'node_modules', 'jsondiffpatch'), {
    '../package.json': './package.json',
    './formatters': './src/formatters/index.js',
    './console': './src/formatters/console.js'
  })
]

@bengotow
Copy link

bengotow commented Jan 3, 2017

For anyone stumbling across this in the future, a discussion around removing the dynamic requires can be found here: #107. Looks like a PR is in order, but will require some testing with webpack, browserfiy, and node.

I'm only using the diff creation and patching parts of jsondiffpatch and didn't need the formatters, so I was able to avoid the problem in this issue by requiring the patcher alone, which doesn't use any dynamic requires:

import {DiffPatcher} from 'jsondiffpatch/src/diffpatcher';
const patcher = new DiffPatcher();
const patch = patcher.diff(a, b); // APIs match docs

@GProst
Copy link

GProst commented Sep 19, 2017

A little adjustment to @gabidobo solution:

    new webpack.ContextReplacementPlugin(/\/src\/(formatters|main)/, ...)

Just added more accurate resource RegExp. With the previous RegExp I had errors with ./node_modules/webpack-dev-server/client/index.js somehow

@epikhighs
Copy link

I tried the context replacement plugin but still get the warning. Does anyone else have luck w/ this? Unfortunately, I'm not directly using jsondiffpatch, but a dependency I'm using requires it so I can't really change how I'm trying to use jsondiffpatch.

WARNING in ./~/jsondiffpatch/src/main.js
61:19-47 Critical dependency: the request of a dependency is an expression
at CommonJsRequireContextDependency.getWarnings (C:\project\digital-ui\node_modules\webpack\lib\dependencies\CommonJsRequireContextDependency.js:27:4)
at Compilation.reportDependencyErrorsAndWarnings (C:\project\digital-ui\node_modules\webpack\lib\Compilation.js:668:24)
at Compilation.finish (C:\project\digital-ui\node_modules\webpack\lib\Compilation.js:531:9)
at C:\project\digital-ui\node_modules\webpack\lib\Compiler.js:486:16
at C:\project\digital-ui\node_modules\tapable\lib\Tapable.js:289:11
at _addModuleChain (C:\project\digital-ui\node_modules\webpack\lib\Compilation.js:477:11)
at processModuleDependencies.err (C:\project\digital-ui\node_modules\webpack\lib\Compilation.js:448:13)
at _combinedTickCallback (internal/process/next_tick.js:73:7)
at process._tickCallback (internal/process/next_tick.js:104:9)

@tony-kerz
Copy link

looks like src folder was removed from npm dist so this doesn't work anymore:

import {DiffPatcher} from 'jsondiffpatch/src/diffpatcher';

@benjamine
Copy link
Owner

benjamine commented Apr 10, 2018

@tony-kerz you shouldn't import from an inner path, can you try:
import {DiffPatcher} from 'jsondiffpatch';?
moreover, if you're trying to get an instance, you can use this to avoid class syntax:

`import jsondiffpatch from 'jsondiffpatch';`
const diffPatcher = jsondiffpatch.create();
const delta = diffPatcher.diff(left, right);

@benjamine
Copy link
Owner

This issue was fixed at 0.3.x by starting to provide pre-built bundles on NPM for the different scenarios (CJS, ES6, ESM).

@tony-kerz
Copy link

@benjamine the following returns null at version 0.3.9:

import jsondiffpatch from 'jsondiffpatch'

but the other incantation you provided here works well, so i'm good, thx again!

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

No branches or pull requests