This repository has been archived by the owner. It is now read-only.

652 - Registry decorator #801

Merged
merged 10 commits into from Dec 21, 2017

Conversation

Projects
None yet
4 participants
@tomdye
Member

tomdye commented Dec 12, 2017

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:

Allows registry items to be defined via a decorator.
Also, Registry will automatically look for a widget constructor as the default import when an esModule is loaded.
Example:

@registry('reg-widget-1', Widget1) // single widget
class TestWidget1 extends WidgetBase {
	render() {
		return w('reg-widget-1', {}); // will render Widget1
	}
}

@registry({
	'reg-widget-1': Widget1,
	'reg-widget-2': Widget2
}) // multiple widgets
class TestWidget2 extends WidgetBase {
	render() {
		return [ w('reg-widget-1', {}), w('reg-widget-2', {}) ];
	} // will render Widget1, Widget2
}

Registry decorator can also accept promises the same way that the standard registry does ie.

@registry('reg-widget-1', () => import('path/to/Widget1')) // single widget
class TestWidget1 extends WidgetBase {
	render() {
		return w('reg-widget-1', {}); // will render Widget1
	}
}

Resolves #652

@tomdye tomdye requested a review from agubler Dec 14, 2017

@@ -164,6 +169,11 @@ export class Registry extends Evented<{}, RegistryLabel, RegistryEventObject> im
this._widgetRegistry.set(label, promise);
promise.then((widgetCtor) => {
if (this._ctorIsDefaultExport<T>(widgetCtor)) {

This comment has been minimized.

@agubler

agubler Dec 14, 2017

Member

Why does this have to be on the registry instance? Could you have done something like this (not on instance)?

export function isWidgetConstructorDefaultExport<T>(item: any): item is ESMDefaultWidgetBase <T> {
   return Boolean(
       module && 
       module.hasOwnProperty('__esModule') && 
       module.hasOwnProperty('default') && 
       isWidgetBaseConstructor(module.default)
   );
}

and used it like this (might have to use a different variable for the assignment):

if (isWidgetConstructorDefaultExport<T>(widgetCtor)) {
    widgetCtor = widgetCtor.default;
}

This comment has been minimized.

@tomdye

tomdye Dec 14, 2017

Member

Thanks @agubler was looking for assistance with these typings

@@ -496,6 +498,14 @@ export class WidgetBase<P = WidgetProperties, C extends DNode = DNode> implement
return dNode;
}
private _runAfterConstructors(): void {
const afterConstructors = this.getDecorator('constructor');

This comment has been minimized.

@agubler

agubler Dec 14, 2017

Member

should we call this afterConstructor?

This comment has been minimized.

@tomdye

tomdye Dec 14, 2017

Member

I aligned it with beforeRenders, afterRenders. it returns an array after all.

This comment has been minimized.

@agubler

agubler Dec 14, 2017

Member

I meant the decorator name, this.getDecorator('constructor') -> this.getDecorator('afterConstructor')

import { RegistryItem } from '../Registry';
export interface RegistryConfig {
[ name: string ]: RegistryItem;

This comment has been minimized.

@agubler

agubler Dec 14, 2017

Member

do we normally add spaces like this for index types?

This comment has been minimized.

@tomdye

tomdye Dec 14, 2017

Member

I thought it was a consistent styling across all single line uses of square brackets

}
/**
* Decorator that can be used to register a widget with the registry

This comment has been minimized.

@agubler

agubler Dec 14, 2017

Member

Perhaps explain that it will register the widget with the widget's local registry?

/**
* Decorator that can be used to register a widget with the registry
*/
export function registry(nameOrConfig: string | RegistryConfig, loader?: RegistryItem) {

This comment has been minimized.

@agubler

agubler Dec 14, 2017

Member

Do you think this would be better overloaded rather than with an optional type? For example if the first argument is RegistryConfig you shouldn't be able to pass a loader argument?

if (typeof nameOrConfig === 'string') {
this.registry.define(nameOrConfig, loader);
} else {
Object.keys(nameOrConfig).forEach(name => {

This comment has been minimized.

@agubler

agubler Dec 14, 2017

Member

Until we move to prettier, at the moment the style guide is for name to be surrounded by parentheses (name)

This comment has been minimized.

@tomdye

tomdye Dec 14, 2017

Member

Not a problem, I thought we'd already dropped brackets for single params.

@agubler

Added some comments

__esModule: boolean;
}
export function isWidgetConstructorDefaultExport<T>(item: any): item is ESMDefaultWidgetBase <T> {

This comment has been minimized.

@agubler

agubler Dec 19, 2017

Member

weird space between ESMDefaultWidgetBase and the generic

This comment has been minimized.

@tomdye

tomdye Dec 19, 2017

Member

won't matter with prettier

This comment has been minimized.

@agubler

agubler Dec 19, 2017

Member

No prettier yet in this repo ;)

/**
* Decorator that can be used to register a widget with the calling widgets local registry
*/
export function registry(nameOrConfig: string, loader: RegistryItem): DecoratorHandler;

This comment has been minimized.

@agubler

agubler Dec 19, 2017

Member

@matt-gadd when we talked about this before, did we decide not to use registry but define as the decorators name?

This comment has been minimized.

@tomdye

tomdye Dec 19, 2017

Member

define gets picked up by webpack according to @matt-gadd , so that's a negatory

This comment has been minimized.

@agubler

agubler Dec 19, 2017

Member

@define gets picked up by webpack? In what way?

This comment has been minimized.

@matt-gadd

matt-gadd Dec 19, 2017

Contributor

@agubler reserved word pretty much for AMD.

@agubler

This comment has been minimized.

Member

agubler commented Dec 19, 2017

Can you add to the description that the changes will now automatically peel the default for all lazy registry items?

@agubler

Are we missing an update in the README regarding the change in behaviour in the registry?

@tomdye tomdye force-pushed the tomdye:registryDecorator branch from 0ce2b2c to 975d983 Dec 19, 2017

@tomdye tomdye force-pushed the tomdye:registryDecorator branch from 975d983 to e5a5128 Dec 21, 2017

@tomdye

This comment has been minimized.

Member

tomdye commented Dec 21, 2017

Raised #810 to track readme update

@tomdye tomdye merged commit b53557e into dojo:master Dec 21, 2017

4 checks passed

codecov/patch 100% of diff hit (target 98.47%)
Details
codecov/project 98.49% (+0.01%) compared to 80216f2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@dylans dylans added this to the beta.5 milestone Dec 22, 2017

@dylans dylans added the beta5 label Dec 22, 2017

@agubler agubler referenced this pull request Jan 18, 2018

Merged

Dojo 2 beta 5 blog post #370

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