aurelia / dependency-injection

A lightweight, extensible dependency injection container for JavaScript.
MIT License
160 stars 66 forks source link

registering instance does not seem to work #83

Closed grofit closed 8 years ago

grofit commented 8 years ago

I had some code I wrote a year or so ago and it seemed to work fine then.

// in the aurelia start
aurelia.container.registerInstance(TemplateGenerator, new DefaultTemplateGenerator());

// in a class which uses this
@inject(..., TemplateGenerator)

This used to work ok and I would get the instance of DefaultTemplateGenerator, however currently with this I just seem to get an instance of TemplateGenerator, in this scenario DefaultTemplateGenerator extends the base class so they should be interchangeable.

EisenbergEffect commented 8 years ago

This certainly seems like a serious bug. Can you create a failing test on the DI repo for us?

grofit commented 8 years ago

Will give it a try, I assumed it was not a bug and just a misuse one my part or it had updated in some way.

EisenbergEffect commented 8 years ago

Looking at the code, it looks like what you show should work...

grofit commented 8 years ago

Just added a test case in a fork:

https://github.com/grofit/dependency-injection/commit/6380fa018622810cb785b045ce2eaff8b2735214

Works as expected, so very confused.

If it helps the codebase I am updating is:

https://github.com/grofit/aurelia-generate

I huge amount of it has changed but is just local atm, so ignore the dependencies and some of the older calls, but the main things to look at are the entry point in index.js which has the definition for the instance (ignore the globalizeresource thats been fixed), then the actual TemplateGenerator classes.

https://github.com/grofit/aurelia-generate/blob/master/src/index.js#L5 https://github.com/grofit/aurelia-generate/blob/master/src/generators/template-generator.js https://github.com/grofit/aurelia-generate/blob/master/src/generators/default-template-generator.js https://github.com/grofit/aurelia-generate/blob/master/src/elements/generate-element.js#L6

(The above should provide all the pertinent bits of code)

If it helps as well the output I get from the generate-element constructor arguments are:

args [generate.au-target, 
ViewSlot { anchor=generate.au-target,  viewAddMethod="appendNodesTo",  animator=Animator,  more...}, 
ViewCompiler { bindingLanguage=TemplatingBindingLanguage,  resources=ViewResources,  compile=compile(),  more...}, 
ViewResources { bindingLanguage=TemplatingBindingLanguage,  parent=ViewResources,  hasParent=true,  more...}, 
TemplateGenerator { generatorType="none",  generateTemplate=function()}]
grofit commented 8 years ago

Is there anything else I can do to debug or help narrow down what the issue is here? I cannot see how it works in the tests but doesn't work in a real world scenario, only difference is I am using @inject decorator for injection.

grofit commented 8 years ago

Ok I think I am going slightly mad, I have tried replicating in isolation in C9, even going as far to make the elaborate local plugin setup and it works as expected much like in the unit test I added in that fork.

However it still refuses to work in the context of the plugin, SO I have uploaded the plugin to c9 and have a "working" instance of this issue, I think you would only get read access by default:

editor: https://ide.c9.io/grofit/aurelia-generate-di-issue demo: https://aurelia-generate-di-issue-grofit.c9users.io/examples/

So when you load it up and check the console, the plugin registers the instance, and there are 3 console logs when this occurs:

normal generator TemplateGenerator {generatorType: "none"}
index.js:16 default generator DefaultTemplateGenerator {generatorType: "default"}
index.js:18 immediate check DefaultTemplateGenerator {generatorType: "default"}

Then when the plugin is used there is an @inject to get the same type, and ends up outputting the wrong type, I am baffled as to why I am getting a TemplateGenerator instance and not a DefaultTemplateGenerator instance, even when it is working as expected earlier on.

I am happy to make any other changes if you want me to test anything on there, as its on a branch but I am baffled as to what is causing it to not resolve correctly. If you want RW access on there just send a request and I will enable it for you.

grofit commented 8 years ago

YES! ok @EisenbergEffect I have managed to replicate it in isolation and its a SUPER strange one.

I have a c9 example of it in action, and it only seems to manifest when you are registering instances which only exist within the plugin.

So for an example of the issue: https://aurelia-injection-issue-grofit.c9users.io/

It you look in the console you will see the dependency is being resolved incorrectly within the plugin.

Then if you go look at the code: https://ide.c9.io/grofit/aurelia-injection-issue

It will show that it should be injecting correctly, but it doesn't. Now the odd thing is, if I take classes outside of the plugin folder and put them into a different route (outside of the scope of the plugin module) it will resolve fine (i.e to start with I had the classes at the root level and worked fine).

If you have any queries feel free to pm me on the gitter I will try to be on it for most of the day today.

EisenbergEffect commented 8 years ago

Is there some way you can reproduce this behavior in the nav skeleton, so I can see it myself? Also, are you sure you aren't somehow getting two separate loads of the same module causing the same classes to be defined (but different instances of those class definitions). Not sure how that would happen, but it could cause this because the keys in the DI, though the same class would actually be different.

grofit commented 8 years ago

That example should be even more bare bones than the nav skeleton app as it only contains the bare minimum stuff (it is based upon the nav skeleton though), I can do the same for that if you wish but it would look about 95% the same just with more code for other pages. I appreciate its a bit of a pain having to view it on c9, but there is no other simple way of really hosting an app that you can fiddle with the source code and see the output. The crux of it is that the plugin is being loaded correctly, but is somehow resetting the container in some way.

EisenbergEffect commented 8 years ago

@grofit We've created a service built on gist. Here's a basic Aurelia setup: https://gist.run/?id=1ae985e6c943885496e8 Would you be able to reproduce it there?

grofit commented 8 years ago

sure thing, will ping you when its done

grofit commented 8 years ago

Hmm this may not be doable as part of the problem seems to be related to where the files live in the folder stack and jspm settings, as this example is all flat. I will give it a go though.

grofit commented 8 years ago

Ok so https://gist.run/?id=e37119c3c0b53bf92241afe74201fc82

However as mentioned there are no folders nor is there any custom jspm config for mappings etc, so although it works in there, in the real world scenario it doesn't... it took me AGES to narrow down into a simple reproduction (which is what the c9 is). So it could be a mix of the jspm config for that plugin being the issue, but however I look at it. I am unable to update the plugins I have as the example scenes which use the plugin just fall over as they get incorrect DI..

grofit commented 8 years ago

After talking briefly with @PWKad it was mentioned that this is not an aurelia issue, however I still feel that although JSPM may be part of the problem it is not solely responsible for the incorrect DI behaviour.

It only exhibits (or at least thats the only scenario I noticed it in) when I am using custom packages and mappings and the registration target lives within the package route, however JSPM has no notion of DI, it purely resolves files, so given all files are resolved and the console logging proves that aurelia is aware of all the types used, its just for some reason the DI state seems to get reset. So given the symptom is within the Aurelia platform and JSPM has done its job as far as module/file resolution goes I still feel that this is something Aurelia related.

Unfortunately I have to use this JSPM package/mapping approach as its the only way I can bundle examples within my plugin repo without having to depend upon a 3rd party host. So until there is a better approach to being able to bundle examples of plugins within the plugin repo I will be unable to change the JSPM config, which means a lot of my plugins will not work due to the DI configuration rules getting reset at some point.

From my primitive research in the area the container used for DI is treated like a singleton and once system.js has done all its resolving (via JSPM) it is all aurelia from that point onwards.

Anyway if anyone has any ideas please let me know, I would raise it with JSPM but given its aurelia that exhibits the problem I doubt they will be able to do much. I am unfortunately blocked on updating my plugins until a workaround/solution is discovered. (Be it using a different JSPM config to resolve local plugin or sorting the DI issue when using this approach)

jdanyow commented 8 years ago

@grofit any chance you could deploy this app somewhere we can debug it?

jdanyow commented 8 years ago

@grofit I just thought of something- is any of your code or your dependencies using Container.makeGlobal()?

grofit commented 8 years ago

@jdanyow nope I only use container.registerInstance and aurelia.globalResource (or globalizeResource whichever it is), its on cloud 9, just click the links listed above and you can play with it and test it without having to deploy anything yourself:

So for an example of the issue: https://aurelia-injection-issue-grofit.c9users.io/

It you look in the console you will see the dependency is being resolved incorrectly within the plugin.

Then if you go look at the code: https://ide.c9.io/grofit/aurelia-injection-issue

I tried to make it as simple as possible for you to just see the basic code, see the output and be able to play with it.

stoffeastrom commented 8 years ago

I just ran it in the debugger and stopped in index.js and saved an reference to the BaseClass function. Then I stopped in the plugin.js and compared them

DEBUG [aurelia] Loading plugin aurelia-injection-issue.
window.INDEXBASECLASS = BaseClass
BaseClass() {
                _classCallCheck(this, BaseClass);

                this.message = "base";
            }
VM428 index.js!transpiled:13 instance before injection ExtendedClass {message: "extended"}
VM372 aurelia-logging-console.js:56 DEBUG [aurelia] Configured plugin aurelia-injection-issue.
INDEXBASECLASS === BaseClass
false

So when injecting the BaseClass in plugin.js it will find the autoRegistered when loading the file which points to a different one than in index.js

No idea why that's happening.. just wanted to share my findings..

grofit commented 8 years ago

There should be console.log calls which already output the above information, but like you say its resolving differently in the plugin and outside of it.

stoffeastrom commented 8 years ago

Yeah I know but this means it's not something in Aurelias D since it's different function references. If they were the same reference it would have worked.

grofit commented 8 years ago

Ah ok so you are proving the resolving type being used is different between the invocations so it would not resolve correctly. So as you say Aurelia DI may not be the source of the problem here, but I don't know where to put the bug, as I dont know if the issue is down to Babel creating different instances during the transpiling phase, or if its JSPM somehow causing them to load in different contexts, or Aurelia seeing them as different types rather than the same.

If you have any idea where the underlying problem would be caused then that would at least help get the right people involved in this, as I mentioned before I have a number of plugins I want to update to the latest version of Aurelia but this problem is keeping me from releasing them, so I hope everyone can agree at this point that this behaviour is "undesired" so hopefully someone will be able to point me to the next step here so I can get the "desired" behaviour.

stoffeastrom commented 8 years ago

I'm not sure why that is happening and would have to dig into it more but I think it can give @EisenbergEffect some Ideas where the problem could be.

EisenbergEffect commented 8 years ago

The only time I have seen this happen is when there were two versions of a particular module being loaded by the module loader. This usually related to forks being present. Other than that, I have no idea. The Aurelia DI just uses a Map under the hood. So if they same key is provided you get back the same value. If it "looks like" the same key but actually isn't, then this usually means the module loader is loading two versions.

grofit commented 8 years ago

Which could very well be the case, so would you say that this is probably a JSPM issue? or to do with the way Aurelia handles plugins?

EisenbergEffect commented 8 years ago

There isn't anything really special about how Aurelia handles plugins. Ultimately, it just makes a request to the loader to loader a module. Did you check to make sure you don't have any forks?

grofit commented 8 years ago

forgive me for being a dunce here but when you say "you don't have any forks?" what is a fork in this context?

EisenbergEffect commented 8 years ago

It's when you have more than one version of the same library installed and being loaded. You can check it with the following command:

jspm inpsect --forks
grofit commented 8 years ago
grofit:~/workspace $ jspm inspect --forks
ok   Install tree has no forks.

Like I say you can take a look yourself at the source and whole "deployed" example via the c9 links if there is anything else specifically you wanted to check.

EisenbergEffect commented 8 years ago

Can you drop a zip in here instead? I'm not too fond of c9 and I'd like to use my local machine to debug and test some stuff? ;)

grofit commented 8 years ago

No problem, c9 lets you download your project anyway so its not a huge ask. You will need to do the normal npm install and jspm install, just dont replace the config file as that is a key part in all this.

https://dl.dropboxusercontent.com/u/98595151/project.zip

grofit commented 8 years ago

Just a quick ping to see if you were able to find anything out with the local version?

EisenbergEffect commented 8 years ago

Not yet. My schedule has been a bit crazy with travelling. I've got the email in my inbox to remind me to look at it soon. Hopefully today or tomorrow.

EisenbergEffect commented 8 years ago

Ok, I'd like to help with this, but after close to 30 minutes, I still can't get the zip above to run properly. It uses a mixture or build compilation and runtime transpilation with different versions of the babel compiler. It looks like there are forks of core-js when you try to install everything. Really, though, I seriously doubt this has anything to do with Aurelia. It looks like it's related to system.js. Aurelia's DI just uses a JS Map internally. So, key to value. If the key is the same, it returns the value. If you've got, somehow, the same library being loaded twice, you could get different versions of the key, causing the lookup to fail and create different instances.

It's hard to get this running with that external plugin folder that isn't getting built but is being transpiled. Versions of the transpilers were mismatching I think. I'm not sue what was happening on my machine, but I didn't succeed in getting it to run. I can't spend any more time on this now. If you can tell me how to set this up properly, that would help.

grofit commented 8 years ago

I dont know what much more I can do for you, I can tell you EXACT thing to do if you want to do your own scenario for it:

import {BaseClass} from "./base-class";
import {InheritedClass} from "./inherited-class";

export function configure(aurelia) {
    aurelia.container.registerInstance(BaseClass, new InheritedClass());
    aurelia.globalResources("./your-element-or-attributet");
}
paths: {
    ...
    "your-plugin-name": "./your-plugin-folder/"
  },
  packages: {
    "your-plugin-name": {
      "main": "index.js",
      "map": {
        "*": "**/*"
      }
    }
  },

This is all I am trying to express in my example, which was originally based off the plugin skeleton example. I appreciate it could be a system.js issue or jspm issue, I will raise it with them to see if one of them fancies helping out, but much like yourself I have expended significant effort trying to just get some code that should work working, I don't get the behaviour expected and based upon conversations and documentation I am using the libraries in the correct way, so there is only so much I can do without having to wade through an ever changing landscape (jspm/systemjs/aurelia).

I am not sure what transpiling issue you were getting, I was using the most compatible versions of babel 5, and like I say c9 has a running working version you can use, it should just work, this is unfortunately the constant headache I get when trying to do any development with aurelia, one of the many tools in the chain changes and it becomes a HUGE mess and most answers to the problem are "remove it all and start again, then import your old files back in". I have tried to make several plugins for aurelia and every step of the way there is some problem, be it jspm, aurelia, babel etc, then I was asked to make examples which I tried to do and even after speaking to your own team many times the answer I got was "just copy " which only lets you use examples if you clone the repo and gulp serve it, which is not a "nice" solution and it means before people even see your plugin in action they have to clone your repo and mess around, so I didn't want to settle for that I wanted to do things in a better way, and that all worked... after HOURS (more like days) trying to find out how to get JSPM to magically realise the local files I have in the repo should be treated like a plugin BOOM I now find that the princess is in another castle (the DI wont work when trying to host the example in a sane way within the repo so people can view it on github without cloning).

I am just ranting now so will end and go cry in a corner for a while, then go see if anyone from JSPM/SystemJS is willing to help diagnose.

EisenbergEffect commented 8 years ago

Trust me, we understand your pain. We are working on our own CLI to help end this insanity. I'll take your above instructions and try it again today. I'll get back to you with some feedback. I do strongly suspect this is something wrong with system.js but I'll test it and let you know for sure.

grofit commented 8 years ago

You are probably right, everything points to system js, its just I dont know if they would assist as I have to use aurelia to exhibit the problem currently, depending on your findings I can at least try to isolate the type differences in a pure JSPM example then see what they say.

If it helps the whole reason this came about was because I wanted to be able to provide an example within the repo, so this meant the gulp serve and the jspm link were out of the question, so the only way I found to be able to get local files to run as a plugin was to use the jspm package/mapping as mentioned above. I did once try to side step this issue by trying to bundle the plugin in with the other aurelia libs but from what I remember it just complained it couldnt find the files.

So if there is another way to solve the above problem that would at least give me a workaround.

https://rawgit.com/grofit/aurelia-chart/master/examples/index.html

Is an example of this hosting the plugin safely on github, without any external stuffs (that plugin doesnt use DI so it doesnt have this issue).

grofit commented 8 years ago

Ok after talking to the JSPM guys it seems like its an issue with the module being loaded twice, as one of the calls has a double slash in the url. It is still being debated if this is a bug or just something a developer has to fix, as it is a perfectly legal url and is being generated by jspm, its just interpreted as a unique one to the existing one.

Anyway will close this one as it was not a DI problem in the first place and now its been shown to be SystemJS. Thanks for your help though.

EisenbergEffect commented 8 years ago

Dang. I thought it might be something like that but didn't know why it was happening. I'm glad you got it sorted and sorry for all the pain this caused. Thanks for the update!