inversify / InversifyJS

A powerful and lightweight inversion of control container for JavaScript & Node.js apps powered by TypeScript.
http://inversify.io/
MIT License
11.3k stars 717 forks source link

Circular dependency in binding_on_syntax.js when compiling using Webpack #963

Open Stnaire opened 6 years ago

Stnaire commented 6 years ago

I got a strange error indicating there is a circular dependency in the library.

Current Behavior

Compilation fails with the following errors:

ERROR in node_modules\inversify\lib\syntax\binding_on_syntax.js -> node_modules\inversify\lib\syntax\binding_when_syntax.js -> node_modules\inversify\lib\syntax\binding_on_syntax.js
ERROR in node_modules\inversify\lib\syntax\binding_when_syntax.js -> node_modules\inversify\lib\syntax\binding_on_syntax.js -> node_modules\inversify\lib\syntax\binding_when_syntax.js

Steps to Reproduce (for bugs)

Simply create a ts file doing an import of the container and a console.log so the compiler do not ignore the import:

import { Container } from "inversify";

console.log(Container);

This, for me, is enough to create the bug.

Stack trace

> webpack --config build/webpack/config/root.config.ts --env.app=native --env.target=localhost --mode=development --watch

webpack is watching the files…

(node:30580) DeprecationWarning: Tapable.plugin is deprecated. Use new API on `.hooks` instead
i 「atl」: Using typescript@2.9.2 from typescript
i 「atl」: Using tsconfig.json from C:/[private]/httpdocs/tsconfig.json
Resolve {%ENV%} vars for file "js/app.js".
Doing a full replacement, the source output is directly modified.
i 「atl」: Checking started in a separate process...
i 「atl」: Time: 729ms
Hash: 419b79590071401b39f1
Version: webpack 4.20.2
Child
    Hash: 419b79590071401b39f1
    Time: 5032ms
    Built at: 2018-10-09 15:52:00
        Asset     Size  Chunks             Chunk Names
    js/app.js  859 KiB     app  [emitted]  app
    Entrypoint app = js/app.js
    [./src/AppBundle/Resources/assets/scripts/ts/native/app.ts] 704 bytes {app} [built]
    [./src/AppBundle/Resources/assets/scripts/ts/native/dependencies.ts] 1.57 KiB {app} [built]
    [./[private]/Resources/assets/app/scripts/ts/native/inversify/container.ts] 2.14 KiB {app} [built]
    [./[private]/Resources/assets/app/scripts/ts/native/modules/jquery/revolution-slider/revolution-slider.ts] 3.05 KiB {app} [built]
    [./[private]/Resources/assets/app/scripts/ts/native/modules/jquery/submit-form/submit-form.ts] 1.88 KiB {app} [built]
    [./[private]/Resources/assets/app/scripts/ts/native/modules/jquery/video-slider/video-slider.ts] 23.6 KiB {app} [built]
    [./[private]/Resources/assets/app/scripts/ts/native/services/jquery-modules-manager/jquery-modules-manager.ts] 5.5 KiB {app} [built]
    [./[private]/Resources/assets/app/scripts/ts/native/services/toastr-renderer/toastr-renderer.ts] 2.34 KiB {app} [built]
    [./[private]/Resources/assets/app/scripts/ts/native/shared/component.ts] 10.7 KiB {app} [built]
    [./[private]/Resources/assets/app/scripts/ts/native/util/doc-ready.ts] 3.12 KiB {app} [built]
    [./[private]/Resources/assets/app/scripts/ts/native/util/extensions/all.ts] 145 bytes {app} [built]
    [./[private]/Resources/assets/app/scripts/ts/native/util/object.ts] 10.2 KiB {app} [built]
    [jquery] external "jQuery" 42 bytes {app} [built]
    [lodash] external "_" 42 bytes {app} [built]
    [reflect-metadata] external "Reflect" 42 bytes {app} [built]
        + 66 hidden modules

    ERROR in node_modules\inversify\lib\syntax\binding_on_syntax.js -> node_modules\inversify\lib\syntax\binding_when_syntax.js -> node_modules\inversify\lib\syntax\binding_on_syntax.js

    ERROR in node_modules\inversify\lib\syntax\binding_when_syntax.js -> node_modules\inversify\lib\syntax\binding_on_syntax.js -> node_modules\inversify\lib\syntax\binding_when_syntax.js
[Browsersync] Proxying: http://some-site.local
[Browsersync] Access URLs:
 ----------------------------
 Local: http://localhost:3001
 ----------------------------
[Browsersync] Watching files...
Stnaire commented 6 years ago

If you look inside "node_modules/inversify/lib/syntax/binding_on_syntax.js" you got on the third line:

var binding_when_syntax_1 = require("./binding_when_syntax");

and in "node_modules/inversify/lib/syntax/binding_when_syntax.js" there is on the third line too:

var binding_on_syntax_1 = require("./binding_on_syntax");

How can it work? I'm really surprised to be the only one having this problem.

For now I'm commenting the whole binding_on_syntax thing so I can compile:

// "use strict";
// Object.defineProperty(exports, "__esModule", { value: true });
// var binding_when_syntax_1 = require("./binding_when_syntax");
// var BindingOnSyntax = (function () {
//     function BindingOnSyntax(binding) {
//         this._binding = binding;
//     }
//     BindingOnSyntax.prototype.onActivation = function (handler) {
//         this._binding.onActivation = handler;
//         return new binding_when_syntax_1.BindingWhenSyntax(this._binding);
//     };
//     return BindingOnSyntax;
// }());
// exports.BindingOnSyntax = BindingOnSyntax;

exports.BindingOnSyntax = function() { };

It doesn't seem to break what I use so it will do for now, but help would be greatly appreciated.

Thanks.

dcavanagh commented 6 years ago

@Stnaire can you please share your webpack config? Or a repo set up to recreate the issue?

Thanks

Stnaire commented 6 years ago

Hi and thanks for your response.

My webpack config is a generic script driven by a YAML configuration file (kind of what I did with a gulp plugin I've made several years ago, gulp-yaml-packages).

But I created a sandbox project and I proceed to remove most of the code to create the simplest demo possible while still using the same code base.

That forced me to read the whole script again and...

private getCommonPlugins(): any[] {
    const output: any[] = [
        [...]
        new CircularDependencyPlugin({
            exclude: /node_modules/,
            onDetected({ paths, compilation }) {
                compilation.errors.push(new Error(paths.join(" -> ")));
            },
        }),
    ];
    [...]
}

I totally forgot about this CircularDependencyPlugin I've added here maybe 1 year ago. And because the error doesn't identify the plugin I didn't think about it...

But that's not enough, I've excluded node_modules in the configuration, so a circular dependency in inversify shouldn't trigger the error.

If I go in the plugin's source code, that's what I find:

for (let module of modules) {
    if (module.resource === undefined) { continue }
    let maybeCyclicalPathsList = this.isCyclic(module, module, {})
    if (maybeCyclicalPathsList) {
        // allow consumers to override all behavior with onDetected
        if (plugin.options.onDetected) {
            try {
                plugin.options.onDetected({
                    module: module,
                    paths: maybeCyclicalPathsList,
                    compilation: compilation
                })
            } catch(err) {
                compilation.errors.push(err)
            }
            continue
        }

        // exclude modules based on regex test
        if (plugin.options.exclude.test(module.resource)) {
            continue
        }
        [...]
    }
    [...]
}

So the isCyclic test is done before checking the exclusion rule, and if the configuration defines a onDetected callback then the error is sent to it.

I had no idea that the exclude test what supposed to be done manually in the onDetected callback, and that's weird.

I've updated the plugin to its latest version and now the code is:

for (let module of modules) {
    const shouldSkip = (
        module.resource == null ||
        plugin.options.exclude.test(module.resource)
    )
    // skip the module if it matches the exclude pattern
    if (shouldSkip) {
        continue
    }

    let maybeCyclicalPathsList = this.isCyclic(module, module, {})
    if (maybeCyclicalPathsList) {
    // allow consumers to override all behavior with onDetected
    if (plugin.options.onDetected) {
        try {
            plugin.options.onDetected({
                module: module,
                paths: maybeCyclicalPathsList,
                compilation: compilation
            })
        } catch(err) {
            compilation.errors.push(err)
        }
        continue
    }
    [...]
}

Now they perform the exclusion test first and it solves the problem.

But still, it seems to me that there is a design problem anyway. Two files shouldn't depend on one another in such a way, a third component should be created to expose the functionality they both depend on.

Thanks for your response, that forced me to investigate deeper.

Adamfsk commented 5 years ago

I'm getting similar warnings when trying to compile using rollup with babel 7:

(!) Circular dependency: node_modules\inversify\lib\syntax\binding_on_syntax.js -> node_modules\inversify\lib\syntax\binding_when_syntax.js -> node_modules\inversify\lib\syntax\binding_on_syntax.js
(!) Circular dependency: node_modules\inversify\lib\syntax\binding_on_syntax.js -> node_modules\inversify\lib\syntax\binding_when_syntax.js ->  C:\Users\modified\library\node_modules\inversify\lib\syntax\binding_on_syntax.js?commonjs-proxy -> node_modules\inversify\lib\syntax\binding_on_syntax.js
DirtyHairy commented 5 years ago

I am getting the same warnings using rollup with typescript:

(!) Circular dependency: node_modules/inversify/lib/syntax/binding_on_syntax.js -> node_modules/inversify/lib/syntax/binding_when_syntax.js -> node_modules/inversify/lib/syntax/binding_on_syntax.js
(!) Circular dependency: node_modules/inversify/lib/syntax/binding_on_syntax.js -> node_modules/inversify/lib/syntax/binding_when_syntax.js -> /Users/pestix/git/6502ts/6502.ts/node_modules/inversify/lib/syntax/binding_on_syntax.js?commonjs-proxy -> node_modules/inversify/lib/syntax/binding_on_syntax.js
davidstellini commented 5 years ago

I have the same issue:

WARNING: Circular dependency: node_modules/inversify/lib/syntax/binding_on_syntax.js -> node_modules/inversify/lib/syntax/binding_when_syntax.js -> node_modules/inversify/lib/syntax/binding_on_syntax.js
authanram commented 3 years ago

@Adamfsk @DirtyHairy @davidstellini this should solve the circular dependency issue;

// rollup.config.js
plugins: [
    commonjs({
        dynamicRequireTargets: [
            'node_modules/inversify/lib/syntax/binding_{on,when}_syntax.js',
        ],
    }),
],

Documentation: https://github.com/rollup/plugins/blob/master/packages/commonjs/README.md#dynamicrequiretargets