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

Linear dependencies between imported modules become circular #65

Closed
demurgos opened this Issue May 11, 2015 · 12 comments

Comments

Projects
None yet
2 participants
@demurgos

Hi,
First of all I would really thank you for this module: if I could star it 10 times, I would! It really saved me a lot of troubles when exporting some common components for node and the client.

I really like the possibility to import general purpose modules (Q, _, custom helpers, etc.) but I feel that I encountered an issue.
I wrote two modules helper.js & _Error.js.
The first one provides some general purpose functions independent of the platform, ie. I included an inherits function (similar to the one in Node's util) so I can use it in the browser too.
The second one extends the native Error to provide more constructors, add data to follow error causes easily in JS, provide easier error naming & output all these data nicely.

Note that _Error extends the native Error with helper.inherits(_Error, Error) so it stays compatible with the test _error instanceof Error. _Error requires helper.

I expect to use these general purpose modules in my whole application so I would like to add both of these modules in my config:

urequire: {
    _defaults: {
        bundle: {
            // ...
            dependencies: {
                imports: {
                    "cmn/js/core/helper": "helper",
                    "cmn/js/core/_Error": "_Error"
                }
        }
    }
}

The problem is that now, helper is required in _Error (that's what I need) but _Error is also required in helper (I don't really need that there, if there's really a problem I'll just use the native Error).
Because both of these modules are imported, their "linear" relation is lost and I get circular dependencies. I cannot really force the order of the imports (I tried: _Error is evaluated before helper, bad luck...) even if I try to explicitly add require("./helper") in _Error to enforce their relation.

In this example, as in many other situations I guess, I could simply copy the code required during the creation of the object to avoid any relation between my modules but I feel there must be a better option.
In this example, I use just helper.inherits during the evaluation of _Error, but if I think that dependencies between general purpose modules and libraries like Q or _ should work automagically like the rest of this module.

I would like to thank you again for this fantastic module and hope that this example can help to improve it. ;)

@anodynos

This comment has been minimized.

Show comment
Hide comment
@anodynos

anodynos May 12, 2015

Owner

Thank you so much for your empowering comments - I would greatly appreciate it if you do spread the word on social media!

I will look into this over this week/weekend & come back to you with an answer!

Owner

anodynos commented May 12, 2015

Thank you so much for your empowering comments - I would greatly appreciate it if you do spread the word on social media!

I will look into this over this week/weekend & come back to you with an answer!

@demurgos

This comment has been minimized.

Show comment
Hide comment
@demurgos

demurgos May 17, 2015

Thank you,
I already encourage my friends to use this module. Once I'll set a blog, be sure I'll write an article on this module, but right now I'm a bit busy with my current projects. I hope to get more free time this summer so I could try to help with some of the open issues.

Thank you,
I already encourage my friends to use this module. Once I'll set a blog, be sure I'll write an article on this module, but right now I'm a bit busy with my current projects. I hope to get more free time this summer so I could try to help with some of the open issues.

@anodynos

This comment has been minimized.

Show comment
Hide comment
@anodynos

anodynos May 17, 2015

Owner

If I understand right you issue is Circular Dependencies in AMD / CommonJs modules. Good news, I think:

  • First of all, it has nothing to do with uRequire (except that uRequire could at least let you know while building, a known issue #46)

*If you really want to have circular dependencies (most times its best to avoid them), *:

  • its solved in CommonJS, you ll find info on the web about 'circular dependencies nodejs' or smthng. But basically you do a) lazy loading of resources or eg like here and/or b) make sure you use the module.exports object already given to you to add properties to it (i.e do this module.exports.doStuff = function(){} and not module.exports = {doStuff: function(){}. You ll be happy to know that your favorite tool (!) supports converting this commonjs when you files convert to AMD/UMD. See also http://urequire.org/masterdefaultsconfig.coffee#build.injectexportsmodule

Now, if you DONT want to have circular dependency, I now realize that the bundle.dependencies.imports will indeed make them both dependent of each other, since they are both part of the bundle, and there is no booleanOrFilespecs for imports.

There is no easy solution for this I fear, unless you a) can make a least one of them a standalone bundle ? But certainly this can be improved. or b) write a custom ResourceConverter for it - see https://github.com/anodynos/urequire-rc-import-keys for example.

If you could please make a github repo with simplistic skeleton of you problem, I will take a closer look.

Regards

Owner

anodynos commented May 17, 2015

If I understand right you issue is Circular Dependencies in AMD / CommonJs modules. Good news, I think:

  • First of all, it has nothing to do with uRequire (except that uRequire could at least let you know while building, a known issue #46)

*If you really want to have circular dependencies (most times its best to avoid them), *:

  • its solved in CommonJS, you ll find info on the web about 'circular dependencies nodejs' or smthng. But basically you do a) lazy loading of resources or eg like here and/or b) make sure you use the module.exports object already given to you to add properties to it (i.e do this module.exports.doStuff = function(){} and not module.exports = {doStuff: function(){}. You ll be happy to know that your favorite tool (!) supports converting this commonjs when you files convert to AMD/UMD. See also http://urequire.org/masterdefaultsconfig.coffee#build.injectexportsmodule

Now, if you DONT want to have circular dependency, I now realize that the bundle.dependencies.imports will indeed make them both dependent of each other, since they are both part of the bundle, and there is no booleanOrFilespecs for imports.

There is no easy solution for this I fear, unless you a) can make a least one of them a standalone bundle ? But certainly this can be improved. or b) write a custom ResourceConverter for it - see https://github.com/anodynos/urequire-rc-import-keys for example.

If you could please make a github repo with simplistic skeleton of you problem, I will take a closer look.

Regards

@demurgos

This comment has been minimized.

Show comment
Hide comment
@demurgos

demurgos Jul 12, 2015

Sorry, I was unavailable for few weeks and then it took me some time to gather the different files to set up the repo.

So, in my case I "DONT want to have circular dependency" so I wrote a resource-converter (it's the first time I tried, it's great !)
I did not really understand how to use "booleanOrFilespecs" but its purpose seems to provide a clean way to whitelist/blacklist imports, right ?

So, here is the repository which highlights my structure and my problem:
https://github.com/Demurgos/urequire-imports-dependencies

I also thought that a solution could be to enforce explicit require for these kind of modules (because they are rare) but it might break some retro-compatibility and would require more work.

Sorry, I was unavailable for few weeks and then it took me some time to gather the different files to set up the repo.

So, in my case I "DONT want to have circular dependency" so I wrote a resource-converter (it's the first time I tried, it's great !)
I did not really understand how to use "booleanOrFilespecs" but its purpose seems to provide a clean way to whitelist/blacklist imports, right ?

So, here is the repository which highlights my structure and my problem:
https://github.com/Demurgos/urequire-imports-dependencies

I also thought that a solution could be to enforce explicit require for these kind of modules (because they are rare) but it might break some retro-compatibility and would require more work.

@anodynos

This comment has been minimized.

Show comment
Hide comment
@anodynos

anodynos Jul 22, 2015

Owner

Thanks so much for this feedback - I will check it out and get back to you, hopefully this weekend :-)

Owner

anodynos commented Jul 22, 2015

Thanks so much for this feedback - I will check it out and get back to you, hopefully this weekend :-)

@anodynos

This comment has been minimized.

Show comment
Hide comment
@anodynos

anodynos Sep 7, 2015

Owner

@demurgos still havent looked at this, due to lack of time... but its in the pipeline & I will get back to you when I have some news :-) Thanks again

Owner

anodynos commented Sep 7, 2015

@demurgos still havent looked at this, due to lack of time... but its in the pipeline & I will get back to you when I have some news :-) Thanks again

@demurgos

This comment has been minimized.

Show comment
Hide comment
@demurgos

demurgos Feb 11, 2017

At this point I don't think that this will be solved. I'll keep my demo repo up for some time but I plan to delete it and close this issue. Please fork it if you think that you might still need it.

At this point I don't think that this will be solved. I'll keep my demo repo up for some time but I plan to delete it and close this issue. Please fork it if you think that you might still need it.

@anodynos

This comment has been minimized.

Show comment
Hide comment
@anodynos

anodynos Feb 14, 2017

Owner

Yeah, so sorry but my time is limited these days and I have unfortunately abandoned the project :-(
I've cloned your repo anyway :-)

Owner

anodynos commented Feb 14, 2017

Yeah, so sorry but my time is limited these days and I have unfortunately abandoned the project :-(
I've cloned your repo anyway :-)

@anodynos

This comment has been minimized.

Show comment
Hide comment
@anodynos

anodynos Feb 16, 2017

Owner

As a side note, circular dependencies are easily solved in CommonJS, if you use the given module.exports object {}, which is automatically passed to you.

So, if instead of reassigning

module.exports = { my: 'own thing' };

you do a

exports.my = 'own thing';

then you will NOT have a problem with circular deps. This should also work on the AMD / UMD urequire conversion.

Of course it doesn't work if you want to assign something other than an {} to exports, such as a function or an array, but still you can attach that function or array to the given {}. Hope this helps!

Owner

anodynos commented Feb 16, 2017

As a side note, circular dependencies are easily solved in CommonJS, if you use the given module.exports object {}, which is automatically passed to you.

So, if instead of reassigning

module.exports = { my: 'own thing' };

you do a

exports.my = 'own thing';

then you will NOT have a problem with circular deps. This should also work on the AMD / UMD urequire conversion.

Of course it doesn't work if you want to assign something other than an {} to exports, such as a function or an array, but still you can attach that function or array to the given {}. Hope this helps!

@anodynos

This comment has been minimized.

Show comment
Hide comment
@anodynos

anodynos Mar 5, 2017

Owner

I've tested with https://github.com/Demurgos/urequire-imports-dependencies and it works like a charm - my tests on the repo are in a bad state and had no time to refactor them :-(

Check the docs here https://github.com/anodynos/uRequire/blob/99c4370464fe0673fe488442ae6fc560ba132565/source/code/config/ResourceConverters.coffee.md#injectdeps

Owner

anodynos commented Mar 5, 2017

I've tested with https://github.com/Demurgos/urequire-imports-dependencies and it works like a charm - my tests on the repo are in a bad state and had no time to refactor them :-(

Check the docs here https://github.com/anodynos/uRequire/blob/99c4370464fe0673fe488442ae6fc560ba132565/source/code/config/ResourceConverters.coffee.md#injectdeps

@demurgos

This comment has been minimized.

Show comment
Hide comment
@demurgos

demurgos Mar 6, 2017

Thank you for fixing this

demurgos commented Mar 6, 2017

Thank you for fixing this

@anodynos

This comment has been minimized.

Show comment
Hide comment
@anodynos

anodynos Mar 6, 2017

Owner

You're welcome @demurgos - there a better fix today with beta.31 - non-bundle dependencies (eg 'lodash') are injected everywhere, only you bundle ones are prevented from being injected in each other (eg 'utils/helpers' and common/config). Check #76 for & http://urequire.org/masterdefaultsconfig.coffee#bundle.dependencies.imports for more :-)

Owner

anodynos commented Mar 6, 2017

You're welcome @demurgos - there a better fix today with beta.31 - non-bundle dependencies (eg 'lodash') are injected everywhere, only you bundle ones are prevented from being injected in each other (eg 'utils/helpers' and common/config). Check #76 for & http://urequire.org/masterdefaultsconfig.coffee#bundle.dependencies.imports for more :-)

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