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

Warning: Critical dependencies. #196

Closed
fresheneesz opened this Issue Mar 11, 2014 · 103 comments

Comments

Projects
None yet
@fresheneesz
Copy link
Contributor

fresheneesz commented Mar 11, 2014

What does this warning mean? Can this warning give a little more information as to its significance and what you might want to do about it (or perhaps where to find more information about it)?

@sokra sokra closed this in c3a242f Mar 11, 2014

sokra added a commit that referenced this issue Mar 11, 2014

allow to configure default RegExps for automatically created contexts
allow to configure when an automatically created context is critical
better warning message in critical dependencies warning

fixes #196
fixes #198
@sokra

This comment has been minimized.

Copy link
Member

sokra commented Mar 11, 2014

added better warning message

@fresheneesz

This comment has been minimized.

Copy link
Contributor Author

fresheneesz commented Mar 11, 2014

Ah, so its a warning that one of the dependencies is an expression. Thanks Sokra!

I would suggest that calling it a "critical dependency" doesn't quite describe what it is. Critical dependency makes me think of a dependency that must exist for things to work. Instead, this is simply warning about a dependency written as an expression. I would leave out the words "Critical dependency" and just let "the request of a dependency is an expression" describe it. Its much more understandable now tho.

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jan 19, 2015

Would there be a way to suppress this warning for 3rd party libraries? I don't want to get this every time I build just because I have a dependency that does this.

@MarkNijhof

This comment has been minimized.

Copy link

MarkNijhof commented Jan 28, 2015

+1 on suppressing

@jhnns

This comment has been minimized.

Copy link
Member

jhnns commented Jan 28, 2015

Would there be a way to suppress this warning for 3rd party libraries? I don't want to get this every time I build just because I have a dependency that does this.

This error should not be ignored. It usually means that every file below a certain path will be included into the bundle (which should certainly not be ignored).

It always needs to be resolved with aliasing or require contexts

@MarkNijhof

This comment has been minimized.

Copy link

MarkNijhof commented Jan 28, 2015

Can I then suggest the error points to these two links?

On Wednesday, January 28, 2015, Johannes Ewald notifications@github.com
wrote:

Would there be a way to suppress this warning for 3rd party libraries? I
don't want to get this every time I build just because I have a dependency
that does this.

This error should not be ignored. It usually means that every file
below a certain path will be included into the bundle (which should
certainly not be ignored).

It always needs to be resolved with aliasing
http://webpack.github.io/docs/configuration.html#resolve-alias or require
contexts
http://webpack.github.io/docs/configuration.html#automatically-created-contexts-defaults-module-xxxcontextxxx


Reply to this email directly or view it on GitHub
#196 (comment).

Mark Nijhof
t: @MarkNijhof https://twitter.com/MarkNijhof
s: marknijhof

@jhnns

This comment has been minimized.

Copy link
Member

jhnns commented Jan 28, 2015

The error message is confusing, yes @sokra.

Unfortunately these problems can not be solved all the same way. It heavily depends on what the library author is trying to achieve, but almost always it can be avoided at all. See #733 and ansman/validate.js#12 (comment)

@MarkNijhof

This comment has been minimized.

Copy link

MarkNijhof commented Jan 28, 2015

But I have no direct influence in how a third party lib does things. Can
this be solved without touching their code? Else a way to supres individual
errors would be great

On Wednesday, January 28, 2015, Johannes Ewald notifications@github.com
wrote:

The error message is confusing, yes @sokra https://github.com/sokra.

Unfortunately these problems can not be solved all the same way. It
heavily depends on what the library author is trying to achieve, but almost
always it can be avoided at all. See #733
#733 and ansman/validate.js#12
(comment)
ansman/validate.js#12 (comment)


Reply to this email directly or view it on GitHub
#196 (comment).

Mark Nijhof
t: @MarkNijhof https://twitter.com/MarkNijhof
s: marknijhof

@kentcdodds

This comment has been minimized.

Copy link
Member

kentcdodds commented Jan 28, 2015

@MarkNijhof, I believe that's what @jhnns is saying. Using aliases, require contexts, or some other mechanism, there is a way to fix the issue. webpack is powerful.

@MarkNijhof

This comment has been minimized.

Copy link

MarkNijhof commented Jan 28, 2015

Ah misunderstood, and yes very happy with webpack

On Wednesday, January 28, 2015, Kent C. Dodds notifications@github.com
wrote:

@MarkNijhof https://github.com/MarkNijhof, I believe that's what @jhnns
https://github.com/jhnns is saying. Using aliases, require contexts, or
some other mechanism, there is a way to fix the issue. webpack is powerful.


Reply to this email directly or view it on GitHub
#196 (comment).

Mark Nijhof
t: @MarkNijhof https://twitter.com/MarkNijhof
s: marknijhof

@sokra

This comment has been minimized.

Copy link
Member

sokra commented Jan 28, 2015

Try the ContextReplacmentPlugin...

@MarkNijhof

This comment has been minimized.

Copy link

MarkNijhof commented Jan 28, 2015

Thanks will try tomorrow :-)

On Wednesday, January 28, 2015, Tobias Koppers notifications@github.com
wrote:

Try the ContextReplacmentPlugin...


Reply to this email directly or view it on GitHub
#196 (comment).

Mark Nijhof
t: @MarkNijhof https://twitter.com/MarkNijhof
s: marknijhof

@ayozebarrera

This comment has been minimized.

Copy link

ayozebarrera commented Jan 19, 2016

I had the same warning using:

let Class = require(url);

and the warning disappeared when I did the following:

let Class = require(`${url}`);

Should I worry about this thing?

Thanks!

@jhnns

This comment has been minimized.

Copy link
Member

jhnns commented Jan 19, 2016

@ayozebarrera that's strange, because your require() statement stays dynamic. There should still be a warning.

Please be aware of that ES2015 modules won't allow dynamic imports, so there's no upgrade path for your code.

@ayozebarrera

This comment has been minimized.

Copy link

ayozebarrera commented Jan 19, 2016

Yeah it's strange because I have dynamic imports in two different places of my code. But the warning only appeared in this last. So checking the other dynamic import I noticed that the difference is the string... and i.e., there's no warnings from webpack ^^

Thanks for the help, and hope my feedback help you to catch any bug!

@fresheneesz

This comment has been minimized.

Copy link
Contributor Author

fresheneesz commented Jan 19, 2016

ES6 modules don't allow dynamic imports? Isn't that a huge problem in cases of circular dependencies?

@jhnns

This comment has been minimized.

Copy link
Member

jhnns commented Jan 20, 2016

Nope. Dynamic imports and circular dependencies are unrelated. ES2015 is able to handle circular dependencies.

@fresheneesz

This comment has been minimized.

Copy link
Contributor Author

fresheneesz commented Jan 21, 2016

@jhnns I take back what I said, I was using dynamic requires to refer to something else. My bad

@brodo

This comment has been minimized.

Copy link

brodo commented Apr 22, 2016

Hi, I just got this error because a library I use does do this. Could you maybe add some explanation to the error message? It was not clear to me that "the request of a dependency is an expression" == "there is a dynamic import here and dynamic imports should not be used".

@mmahalwy

This comment has been minimized.

Copy link

mmahalwy commented May 26, 2016

Getting it with:

const loadRouteAsync = path => (nextState, cb) => {
  return System.import(path).then(module => {
    console.log(module);
    cb(null, module)
  });
};
@jhnns

This comment has been minimized.

Copy link
Member

jhnns commented May 27, 2016

@mmahalwy The code you're using is too generic, webpack needs to include all files inside the current folder and all files in child folders. Try to make the path more explicit, like System.import("pages/" + path). Webpack has to guess in advance what kind of module you're trying to import by analyzing your source code. If your source code gives no clue, webpack makes all modules in this folder importable.

@seiyria

This comment has been minimized.

Copy link

seiyria commented Jun 8, 2016

So one of the third party libraries I'm using (Tabletop) does this:

  if (typeof process !== 'undefined' && !process.browser) {
    inNodeJS = true;
    var request = require('request'.trim()); //prevents browserify from bundling the module
  }

Does Webpack bundle anything extra as a result, or what should I do to tell Webpack to not make any attempts for this, etc, because this require simply does not need to be resolved.

@ericmorand

This comment has been minimized.

Copy link

ericmorand commented Aug 17, 2018

Could Webpack stop considering developers as dumb people who don't know what they are doing? If I want to include a dependency based on an expression, or if a third-party library want to do that, since this is totally supported by nodejs require, then Webpack should not warn about anything.

There is nothing wrong here, so stop warning about things that work.

@hedgepigdaniel

This comment has been minimized.

Copy link
Contributor

hedgepigdaniel commented Sep 1, 2018

It's good to have this warning on by default IMO, and not a good idea to turn off the warning.
I can't think of a use case where you would need to import something that is determined at runtime, AND there is no finite list of things you might want to import that is known at compile time.

Instead of

//     This path is dynamic, and there will be a warning
//                         |
//                         v
import(`./languages/${language}`).then(() => {/* ... */}))

You can always do

const importLanguage = (language) => {
  switch (language) {
    case 'en': {
      return import('./languages/en');
    }
    case 'pt': {

      //    This path is static! There is no warning!
      //                     |
      //                     v
      return import('./languages/pt');
    }
    // ...etc
    // We know the list of languages, because we created a source file for each one!
  }
};

importLanguage(language).then(() => {/* ... */});

Some problems with importing dynamically generated paths:

  • You might unintentionally compile code that is not intended to be imported or even code that is not referenced by your project. e.g. in the example above you might end up compiling ./languages/helpers/toLowerCase.js or even ./languages/.thumbnails.json
  • Private information that you did not intend to share might be available publicly
  • If you are not code splitting on the import, your chunks might be larger than they need to be and runtime performance will suffer
  • Whether you are code splitting or not, compilation speed will suffer. Try putting import(`${someVariable}`) in a big project and see how long it takes.

Unfortunately specifying all possible import paths is a bit more verbose, but it avoids a heap of issues which might not be obvious how to diagnose and fix.

@Pauan

This comment has been minimized.

Copy link

Pauan commented Sep 1, 2018

@hedgepigdaniel The most common situation where I get the critical dependencies warning is when I use an npm library which does runtime checks to determine whether it is running in Node or not (and if it is, it then loads some other module).

Since it's in a library, I obviously cannot control it, and the library's use case (dynamically loading a module only on Node) is completely valid.

@ericmorand

This comment has been minimized.

Copy link

ericmorand commented Sep 1, 2018

@hedgepigdaniel

I can't think of a use case where you would need to import something that is determined at runtime, AND there is no finite list of things you might want to import that is known at compile time.

This is not the purpose of a bundler to take that kind of decision. There may be a valid use case or not.

might not be obvious how to diagnose and fix.

That's exactly my point: as developers, we know what we are doing and webpack should assume that. Warning about things that may be code smell is baby-sitting. Just like Rollup that warns about eval usage which is "strongly discouraged". This is laughable. eval is a perfectly valid function which usage is not discouraged. It is not recommended if the eval'd string is from unsafe origin (typically a user input or a non-secure remote origin) but if I use eval to execute a code that I control totally, me, the developer, then it is totally safe. Warning about this is baby-sitting.

@hedgepigdaniel

This comment has been minimized.

Copy link
Contributor

hedgepigdaniel commented Sep 2, 2018

Since it's in a library, I obviously cannot control it, and the library's use case (dynamically loading a module only on Node) is completely valid.

You can fork the library if you want, or ask the maintainers to fix it.

Sure, importing different things on node/web is a perfectly fine thing to do, but there is no reason why you would need to import something from a dynamic path. If the library has an if statement and imports different things on each side, you would not see this warning. It's fine to dynamically decide which import statements to execute - the warning is caused by having any particular import statement which might import different things depending on some variable.

baby sitting

You can turn off the warning: https://webpack.js.org/configuration/module/#module-contexts

@arush

This comment has been minimized.

Copy link

arush commented Sep 2, 2018

Having the same issue as @nickjanssen in CRA and prettier (because of graphql-playground-react)

Is there any solution without having much control over the webpack config?

@nicbarker

This comment has been minimized.

Copy link

nicbarker commented Jan 7, 2019

I think it's worth mentioning here that this comment by @jhnns 3 years ago is no longer correct:

Please be aware of that ES2015 modules won't allow dynamic imports, so there's no upgrade path for your code.

There is indeed a proposal in stage 3 for dynamic import statements.

https://github.com/tc39/proposal-dynamic-import

@9ae8sdf76

This comment has been minimized.

Copy link

9ae8sdf76 commented Feb 10, 2019

@hedgepigdaniel It's also worth noting that writing an Electron app that uses plugins would likely store said plugins in the userData folder, which would be different for every user; unknowable at compile time.

@hedgepigdaniel

This comment has been minimized.

Copy link
Contributor

hedgepigdaniel commented Feb 11, 2019

@9ae8sdf76 If you wanted to dynamically load plugins at runtime, I think you would need to use externals. Otherwise, webpack would attempt to compile away the import/require statement and the bundle would not import plugins at runtime. Using externals, I don't think you would see this warning.

@9ae8sdf76

This comment has been minimized.

Copy link

9ae8sdf76 commented Feb 11, 2019

@hedgepigdaniel I'm not sure I understand if externals would work for my use-case.

🍿Backstory🍿

Ideally, I'd like my Electron app to have an HTML interface that would allow users to search npmjs or github for published plugins that my app supports. When the user clicks Install for any given plugin(s), the corresponding release of the plugin would be downloaded into the user's userData folder (e.g., on a Mac, it'd be /Users/<username>/Library/Application Support/AppName/(node_modules|plugins)/<plugin>. Once downloaded, the plugin would be loaded into the running application, and an entry would be written to the application's config.json file that lives in the same userData folder so that the plugin configuration can persist between restarts. At no point should the Electron app, or plugin, be re-compiled. It should Just Work(tm).

I feel it is unfair to assume that I could possibly ever know every single plugin that may possibly exist now, or into the future. And even if I somehow could keep track of all of the plugins, that would mean including them all into the Electron app at compile time, which would cause tremendous bloat (which would continue to increase over time as more plugins would [hopefully] exist). It would also mean that anytime a plugin was updated, the entire Electron app would need to be recompiled, and that is certainly unacceptable. The only time the Electron app should be updated is when a new version of the app itself is released; not because a third-party plugin is updated.

With all of that said, hopefully we're on the same page so far.

If I'm understanding the linked documentation, are you suggesting that I could write a function (for example) that could detect my dynamic import call, but without having to know exactly what is going to be require()'d at runtime, to tell webpack not to compile away the require statement? It's also very important that I not have to somehow define the app's userData directory either, because well, that'd be impossible. :)

@hedgepigdaniel

This comment has been minimized.

Copy link
Contributor

hedgepigdaniel commented Feb 11, 2019

I think you need to check the documentation for externals and do some experimentation to work out how to make your app dynamically require these plugins at runtime - I'm sure its possible.

AFAICT, your issue is unrelated to this warning, and if you see this warning its because for some reason webpack is trying to resolve the plugins at compile time, which is the opposite of what you want.

@9ae8sdf76

This comment has been minimized.

Copy link

9ae8sdf76 commented Feb 14, 2019

@hedgepigdaniel The thing is, OP's error is exactly what brought me here. I'm using a function that essentially does the following (I'm not in front of my code right now):

import { basename, dirname } from 'path';
import Module from 'module';

module.exports = function (filepath) {
    const base = basename(filepath);

    if (base !== filepath) {
        const dir = dirname(filename);
        const m = new Module(dir);

        m.filename = filename;
        m.paths = Module._nodeModulePaths(dir);
        return m.require(base);
    }

    return require(base);
};

How would I tell webpack not to resolve this at compile time?

@Pauan

This comment has been minimized.

Copy link

Pauan commented Feb 14, 2019

@9ae8sdf76 Doing something like this should work:

import { basename, dirname } from 'path';
import Module from 'module';

module.exports = function (filepath) {
    const base = basename(filepath);

    if (base !== filepath) {
        const dir = dirname(filename);
        const m = new Module(dir);

        m.filename = filename;
        m.paths = Module._nodeModulePaths(dir);
        return m.require(base);
    }

    const r = require;
    return r(base);
};

Just so long as you aren't literally using require(...) then it'll fool Webpack.

@9ae8sdf76

This comment has been minimized.

Copy link

9ae8sdf76 commented Feb 15, 2019

@Pauan that was the missing link. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment