Support Injector instances in Registry and add inject decorator #668

agubler · 2017-09-10T09:26:33Z

Type: feature

The following has been addressed in the PR:

There is a related issue
All code matches the style guide
Unit or Functional tests are included in the PR

Description:

Adds inject decorator to support side loading properties from a Injector instance registered in the Registry.

Includes:

Adds additional API for define, get and has that deal with Injector instances (defineInjector, getInjector, hasInjector)
Add inject decorator that registers a function that will be called with the injector instances context and the widget properties in the beforeProperties phase.
Updates Container & Themeable to use mixin pattern for adding inject decorator
Removes BaseInjector that extends WidgetBase the Injector HOC
Renames WidgetBase.getRegistries() to WidgetBase.registries

Resolves #648

kitsonk · 2017-09-11T07:50:22Z

src/Injector.ts

-	private _context: T;
-
-	constructor(context: T = <T> {}) {
+	constructor(context: any) {


What was the reason/motivation for the context being untyped? Not challenging, just wondering.

There was a challenge with the typing, I'll try it again and let you know (can't remember off the top of my head).

Also wasn't sure what the benefit really was, seen as it's injected at run time there's it didn't really give much to the consumer. The important typing is really on the getProperties function that gets passed the context, which the user can type as needed.

I was imagining the challenge it seems, updated to accept a generic type.

I do get the point about the value of it...

kitsonk · 2017-09-11T07:51:21Z

src/Registry.ts

@@ -29,31 +30,55 @@ export interface RegistryEvents extends BaseEventedEvents {
 /**
 * Widget Registry Interface
 */
-export interface Registry {
+export interface RegistryInterface {


We have traditionally not mentioned Interface in our interfaces. Why the change?

Did it have something to do with name merging? Could this be a use case for an abstract class and having an AbstractRegistry as an export?

The reason was, that without a different name for the interface TS didn't actually error if the class didn't implement it! We've used a suffix of Interface in some of the interfaces exported from interfaces.d.ts which was the motivation.

I don't think Abstract works because the registry isn't actually abstract?

Makes sense. Thanks.

kitsonk · 2017-09-11T07:54:13Z

src/decorators/inject.ts

+	getProperties: GetProperties;
+}
+
+export function inject({ name, getProperties }: InjectConfig) {


Some basic JSDoc?

matt-gadd · 2017-09-11T13:04:14Z

src/Registry.ts

+	public getInjector<T extends Injector>(label: RegistryLabel): T | null {
+		if (!this.hasInjector(label)) {
+			if (this.has(label)) {
+				console.warn(`Unable to find injector '${label.toString()}', did you mean to get a widget?`);


i'm not convinced we need these warns, because the api is separate and returns discreet types (that extend Injector), the user thus shouldn't be able to get this wrong at design time.

matt-gadd · 2017-09-11T13:05:23Z

src/decorators/inject.ts

+export function inject({ name, getProperties }: InjectConfig) {
+	return handleDecorator((target, propertyKey) => {
+		beforeProperties(function(this: WidgetBase, properties: any) {
+			const context = this.registries.getInjector(name);


should this be called injector, over context?

yup good shout

matt-gadd · 2017-09-11T13:05:53Z

src/decorators/inject.ts

+ * used to map the injected properties.
+ */
+export interface GetProperties<T = any> {
+	(context: any, properties: T): T;


is this context?

this is the context from the Injector

what is a context? can we get a better name?

I am receptive to better names.

I can't participate in suggesting a name without knowing what it is 😄

If you want it to be payload, I don't have any strong attachment to context or objections to payload - they both work.

the reason i don't like context is the baggage that comes from react with it

Yup I understand that. Let me update that for you - I think both are okay and if using context could lead to confusing that is it enough reason to change.

value? Since you are injecting a value of some sort that can be any. Do we actually expect it to be any or is it an object?

Aye it can be a any, just as the payload of an Injector can be.

matt-gadd · 2017-09-11T13:07:05Z

src/Container.ts

-	return class extends WidgetBase<any> {
+	@inject({ name, getProperties })
+	class WidgetContainer extends WidgetBase<Partial<W['properties']>> {
+		public __setProperties__(properties: Partial<W['properties']>): void {


have we thought how we can better support this in the future? this is the equivalent of shouldComponentUpdate set to true in react?

I have thought about it, shall raise an enhancement issue - for now this is the only way I could support it.

matt-gadd · 2017-09-11T13:10:16Z

src/WidgetBase.ts

@@ -706,7 +706,7 @@ export class WidgetBase<P = WidgetProperties, C extends DNode = DNode> extends E
 			let child: WidgetBaseInterface<WidgetProperties>;

 			if (!isWidgetBaseConstructor(widgetConstructor)) {
-				const item = this.getRegistries().get(widgetConstructor);
+				const item = this.registries.get(widgetConstructor);


is this the only place the registries are accessed? just noticed that we've renamed it back now.

I think everywhere else actually accesses the private (_registries) in WidgetBase, I'll update this to do that.

Support Injector instances in Registry and add inject decorator

aed0ec3

agubler requested review from kitsonk, rorticus and matt-gadd September 10, 2017 09:26

dylans added this to the 2017.09 milestone Sep 10, 2017

kitsonk reviewed Sep 11, 2017

View reviewed changes

agubler added 3 commits September 11, 2017 09:50

Support generic typing for injector

93df2d1

add jsdoc

f077004

ensure test is covering the specified scenario

ab323d4

matt-gadd reviewed Sep 11, 2017

View reviewed changes

remove runtime warns

b06081f

matt-gadd reviewed Sep 11, 2017

View reviewed changes

agubler added 3 commits September 11, 2017 14:11

rename context to injector

f201460

use the private attribute, as per other uses in WidgetBase

e6272e6

rename context to payload

dd5fd86

matt-gadd approved these changes Sep 11, 2017

View reviewed changes

agubler merged commit 6779ca9 into dojo:master Sep 12, 2017

Support Injector instances in Registry and add inject decorator #668

Support Injector instances in Registry and add inject decorator #668

Uh oh!

Conversation

agubler commented Sep 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agubler Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agubler Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

agubler Sep 11, 2017 •

edited

Loading

agubler Sep 11, 2017 •

edited

Loading