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

Fixes DI for aurelia beta 1 #22

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

tkhyn
Copy link

@tkhyn tkhyn commented Dec 5, 2015

A slightly different approach from #21, for which I had most of the work done before (except the tests that I just added).

Things to note:

Compared to #21:

  • no need for an InstanceDispatcher proxy, the Dispatcher instances can be linked against the instances directly. dispatchOwn and registerMetadata could be '_ed' though.
  • each time DispatcherResolver.get(container) is invoked, a new Dispatcher instance is created. This instance may or may not be linked against the target instance when Dispatcher.connect is called, depending on Metadata. If it ends up not being referenced, it'll be garbage-collected.
  • as a consequence, and as a target instance can only be linked to one Dispatcher instance, there is no need to check for the existence of instance[Symbols.instanceDispatcher] before connecting the target instance to its Dispatcher instance

Compared to Rob's suggestion quoted in #17:

  • I did not see the need to use an Array instead of _lastDispatcher. If there is a use case where it'd actually be needed feel free to submit it! EDIT: finally it was needed
  • Instead of getting an InstanceDispatcher singleton from the container, we retrieve a new Dispatcher instance each time. See above.

Ideas I had while making the changes for possible improvements from here:

  • automatically register a Dispatcher instance with an instance even if the dependency has not been injected (for instances that are only used as stores for example). But that's a 'nice to have'.
  • add tests for handle / dispatch / waitFor

Comments welcome!

EDITs 6 Dec: corrections

@tkhyn tkhyn mentioned this pull request Dec 5, 2015
@airboss001
Copy link

I think I found a couple of gotcha's.

You have to have each class import Dispatcher with DI, if you just use {handle} alone in a class that just wants to receive notifications, it will never find the target on dispatch. Maybe that is by design, but wasn't clear and I went round and round on that one trying to figure out why I wasn't getting updates.

Second, you cannot pass a dispatcher from one class to another class when you new it up.
It again fails to find the @handle decorators and never calls it. This worked before, and I think is related to the item noted above.

Basically I have a container class new up collection objects that need notifications to do some recalculations, and classes with only @handle, for my purposes I need to able to wire up the @handle when created, not just when Dispatcher is injected.

Does that make sense? Is there an alternative without having to hard connect into the DI container to create objects so that they get wired up? Am I missing something and this is easier then I am thinking?

@tkhyn
Copy link
Author

tkhyn commented Dec 6, 2015

Thanks for the feedback, I was indeed wrong in assuming that in previous versions @handle could only work in instance of classes were Dispatcher was injected. My mistake, I did not explore that kind of use case with the previous version.

Now I can see what is happening here, actually (and you should experience the same issues with #21, btw). There is a quick-and-dirty fix I can think of, I'll keep for later the more elegant solution I had in mind to enable @handle without requiring the Dispatcher to be injected in the first place.

@airboss001
Copy link

I appreciate your effort and time spent on this, thanks so much!

@tkhyn
Copy link
Author

tkhyn commented Dec 6, 2015

Done, even if the original idea was to monkey-patch invoke only where it's needed and here we end up monkey-patching it everywhere. There's clearly a cleaner solution but that'd require bigger changes and this can be kept for later.

This time I've tested it against Tomasz's aurelia-flux-todo, which I've forked to make it work with aurelia 1b1 and this PR: tkhyn/aurelia-flux-todo

@airboss001
Copy link

Wow, awesome, that was fast! I had just pulled up the handle points into the top container, but easy enough to revert and test this real quick.

@airboss001
Copy link

hmm...so I pulled from master and no change here. I see the dispatch of the message, but the classes that do not have the dispatcher get no @handle notifications. I stepped through a bit and don't see those classes listed in the map either. Let me see if I can figure out how to make a plunker. I will update here when I do.

@airboss001
Copy link

Here is a pared down example. https://github.com/airboss001/aurelia-plunker
I haven't figured out how to get it into plunker, I think I have to copy every file but this should give you an idea of what I am doing.

@tkhyn
Copy link
Author

tkhyn commented Dec 7, 2015

Ok, I better understand what you meant in the 2nd part your first message (and my modifications were actually intended to address the 1st part only).

For the decorators to work, they must decorate methods of a class that is injected as a dependency (= a singleton instance). Here, you are trying to decorate 'normal' classes, that can be instantiated multiple times. What I understand is that you would like every instance of this class (possibly a relatively high number of objects) to execute the code in that method each time the "update" event is dispatched. I'm not sure you really want that, as this is likely to result in very poor performance each time an event is fired (looping over the registry + checking the handlers + calling the method on every single instance).

Decorators are intended to be used in stores only (i.e. classes that are intended to be instantiated only once and injected in ViewModels). This is normal behavior if they don't work for 'normal' classes. As it can be expected, your example does not work with previous versions of aurelia-flux either.

Sorry to have given you false hopes, but I'm afraid you'll have to pull back all your handling method in the store!

@airboss001
Copy link

I could have swore it worked that way before (I could very well be wrong), but I understand what your saying.
I guess I never picked up that it was only for stores, and not general classes. Been awhile since I last looked at it in that depth, and how I had previously used it as a lot of changes since then.

I have pulled up the notification into the parent and loop through the collections calling an update function, just seems more convoluted. I guess EA wasn't that much better and is probably less performant if you had large collections.

Thanks Thomas for your work on this!

@airboss001
Copy link

I am finding some odd behavior on bundling this up. Some classes appears to lose the injected dispatcher that works on constructor call, but the next call I end up getting the undefined object error on dispatch.
I will see if I can create a demo project that shows this behavior as soon as I can.

Update: apparently there was a npm or jspm package that wasn't updated. After reinstall everything is working normal.

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

Successfully merging this pull request may close these issues.

2 participants