Functional Factory Injector #910

agubler · 2018-04-16T18:08:57Z

Type: feature

The following has been addressed in the PR:

There is a related issue
All code has been formatted with prettier as per the readme code style guidelines
Unit or Functional tests are included in the PR
Any required updates included in the readme

Description:

It has been identified that the ergonomics around using an Injector class as the base type when defining an injector in a registry can be awkward.

It requires consumers to deal with Evented.emit(), unlike everywhere else in Dojo 2, where we tend to provide abstractions on top of listening and emitting events.
Additionally it was awkward to just provide a static payload that never needed to be reset.
It encouraged consumers to extend the Injector class and make it stateful which was never the intention.

This change moves to accepting a functional factory that gets provided an invalidator and returns a function that when called will return the intended payload to be injected into the registered widgets.

registry.defineInjector('state', (invalidator: () => void) => {
    const applicationContext = new ApplicationContext(invalidator);
    return () => applicationContext;
});

If the payload never changes then the invalidator function can just be ignored.

registry.defineInjector('state', () => {
    return () => ({ foo: 'bar' });
});

Resolves #911

dylans

A few minor readme updates, very much looking forward to this change landing, thanks!

dylans · 2018-04-16T21:34:51Z

README.md

@@ -851,18 +854,32 @@ class MyClass extends WidgetBase {

 ### Containers & Injectors

-There is built-in support for side-loading/injecting values into sections of the widget tree and mapping them to a widget's properties. This is achieved by registering a `@dojo/widget-core/Injector` instance against a `registry` that is available to your application (i.e. set on the projector instance, `projector.setProperties({ registry })`).
+There is built-in support for side-loading/injecting values into sections of the widget tree and mapping them to a widget's properties. This is achieved by registering an injector factory with a `registry` and setting the registry as a property on the application's `projector` to ensure that is available to your application.


"to ensure that is available" please be more specific than "that is"

dylans · 2018-04-16T21:35:36Z

README.md


-Create an `Injector` instance and pass the `payload` that needs to be injected to the constructor:
+The injector factory gets passed an `invalidator` function that can be called when something has changed that requires connected widgets to `invalidate`.


can be called -> can get called

dylans · 2018-04-16T21:35:49Z

README.md

-registry.defineInjector('my-injector', injector);
+registry.defineInjector('my-injector', (invalidator) => {
+    // This could be a store, but for this example is simply an instance
+    // that accepts the injector and calls it when any of it's internal


it's -> its

maier49

Nice 👍 . Saw one typo and had one small question/suggestion.

maier49 · 2018-04-17T17:02:28Z

README.md


-`getProperties` receives the `payload` from an `injector` and `properties` from the container HOC component. These are used to map into the wrapped widget's properties.
+`getProperties` receives the `payload` return from the injector function and the `properties` passed to the container HOC component. These are used to map into the wrapped widget's properties.


return -> returned

maier49 · 2018-04-17T17:09:22Z

src/Registry.ts

@@ -152,7 +157,7 @@ export class Registry extends Evented<{}, RegistryLabel, RegistryEventObject> im
 		}
 	}

-	public defineInjector(label: RegistryLabel, item: Injector): void {
+	public defineInjector(label: RegistryLabel, item: InjectorFactory): void {


Since this is no longer the item that actually ends up in the registry, maybe it'd be clearer to just name this injectorFactory, or injector?

I can defo change that

agubler · 2018-04-18T11:33:38Z

@maier49 Think i've addressed you're feedback 👍

devpaul · 2018-04-18T15:57:54Z

src/Registry.ts

 		if (!this._injectorRegistry || !this.hasInjector(label)) {
 			return null;
 		}

-		return this._injectorRegistry.get(label) as T;
+		return this._injectorRegistry.get(label)!;


In the return were InjectorItem<T> | undefined wouldn't we be able to avoid the !?

We already check whether the item exists, so we know at this point it is not undefined and I wanted to avoid changing the API any more than required.

devpaul · 2018-04-18T16:08:46Z

src/decorators/inject.ts

 				const registeredInjectors = registeredInjectorsMap.get(this) || [];
 				if (registeredInjectors.length === 0) {
 					registeredInjectorsMap.set(this, registeredInjectors);
 				}
-				if (registeredInjectors.indexOf(injector) === -1) {
+				if (registeredInjectors.indexOf(injectorItem) === -1) {


Can we use a WeakSet<injectorItem> here?

devpaul

This is a marked improvement over extending Injector 💯 Had a couple questions related to implementation. The architecture is sound

agubler added 2 commits April 16, 2018 13:36

Use factory for injector

d90d203

uncomment tests

e84884a

agubler added the breaking-change label Apr 16, 2018

agubler added this to the 2.0.0 milestone Apr 16, 2018

agubler requested review from devpaul and maier49 April 16, 2018 18:09

README

37e0c5b

dylans suggested changes Apr 16, 2018

View reviewed changes

README

232c7be

dylans approved these changes Apr 17, 2018

View reviewed changes

maier49 reviewed Apr 17, 2018

View reviewed changes

agubler added 2 commits April 18, 2018 12:31

address feedback

1a12edb

address feedback

144087c

maier49 approved these changes Apr 18, 2018

View reviewed changes

devpaul reviewed Apr 18, 2018

View reviewed changes

devpaul suggested changes Apr 18, 2018

View reviewed changes

devpaul approved these changes Apr 18, 2018

View reviewed changes

agubler merged commit 9d9c2ee into dojo:master Apr 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Functional Factory Injector #910

Functional Factory Injector #910

agubler commented Apr 16, 2018 •

edited

Loading

dylans left a comment

dylans Apr 16, 2018

dylans Apr 16, 2018

dylans Apr 16, 2018

maier49 left a comment

maier49 Apr 17, 2018

maier49 Apr 17, 2018

agubler Apr 17, 2018

agubler commented Apr 18, 2018

devpaul Apr 18, 2018

agubler Apr 18, 2018 •

edited

Loading

agubler Apr 18, 2018

devpaul Apr 18, 2018

devpaul left a comment


		Create an `Injector` instance and pass the `payload` that needs to be injected to the constructor:
		The injector factory gets passed an `invalidator` function that can be called when something has changed that requires connected widgets to `invalidate`.


		`getProperties` receives the `payload` from an `injector` and `properties` from the container HOC component. These are used to map into the wrapped widget's properties.
		`getProperties` receives the `payload` return from the injector function and the `properties` passed to the container HOC component. These are used to map into the wrapped widget's properties.

Functional Factory Injector #910

Functional Factory Injector #910

Conversation

agubler commented Apr 16, 2018 • edited Loading

dylans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maier49 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agubler commented Apr 18, 2018

Choose a reason for hiding this comment

agubler Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devpaul left a comment

Choose a reason for hiding this comment

agubler commented Apr 16, 2018 •

edited

Loading

agubler Apr 18, 2018 •

edited

Loading