@inject fails silently when Injector is missing from the registry

[devpaul]

Bug

When the inject decorator is missing an injector in the registry it skips calling getProperties

if (injector) {
	const registeredInjectors = registeredInjectorsMap.get(this) || [];
	if (registeredInjectors.length === 0) {
		registeredInjectorsMap.set(this, registeredInjectors);
	}
	if (registeredInjectors.indexOf(injector) === -1) {
		injector.on('invalidate', () => {
			this.emit({ type: 'invalidated', target: this });
		});
		registeredInjectors.push(injector);
	}
	return getProperties(injector.get(), properties);
}

Essentially this allows injection to fail silently and doesn't allow getProperties to react. Instead, it would be useful to pass undefined to getProperties and allow it to handle an unregistered injector. It could then

• throw an error
• provide default property values
• choose to ignore it as we do today

This is especially important for Container because it transforms all of a Widget's properties into optional. So while I may have a Widget where all of it's properties are required, when @Inject skips a call to getProperties I don't have an opportunity to deal with the missing properties and my Widget will expect for those properties to exist due to its properties typings.

Code

interface MyWidgetProperties {
    descriptor: { name: string };
}

class MyWidget extends WidgetBase<MyWidgetProperties> {
    protected render() {
        return v('div', {
            'data-name': this.properties.descriptor.name;
        });
    }
}

const MyWidgetContainer = Container(MyWidget, 'state', {
    getProperties(state) {
        descriptor = state ? state.descriptor : { name: '' };
        return {
            descriptor
        };
    }
});

Expected behavior:

The mapper would provide a default descriptor to the contained widget

Actual behavior:

render() throws an error when 'state' does not exist in the registry.

[agubler]

I think perhaps a warn in the console would be enough for instances when the injector item cannot be found.

[devpaul]

A console.warn is nice, but what about calling getProperties?

[agubler]

I don't think we should call getProperties if there is no external data to inject.

[devpaul]

What about Container's responsibility to provide properties to the injected Widget? If you don't call getProperties there's no opportunity for defaults to be provided. Then we're back to making all MyWidgetProperties optional and handling within MyWidget to ensure a proper render().

[agubler]

I don't think getProperties should ever have to deal with the external state not being available, if the external state isn't in the registry then it's almost certainly an oversight (bug) so my thinking was a console.warn would be enough to tell the consumer that they haven't set the registry item correctly (or used the wrong registry label)

[devpaul]

If it's truly a bug, a console.warn doesn't help aggregate these errors to a central logging server when in production or help resolve Container's missing property data.

[agubler]

It's more of a helper in development, to point them to the correct direction to understand why things aren't working as expected, I guess we could throw an error so the application actually fails.

Basically I wouldn't expect this to ever get to production as the application just wouldn't work, right?

[devpaul]

If an Injector is registered asynchronously or the state comes externally (i.e. state is passed to a custom element via a property) it's possible that a race condition makes things look OK in local development and fail in production. Not to say the architecture couldn't be built more appropriately or that if I thought long enough about it I could come up with a better scenario.

In the general case where getProperties doesn't check for undefined then their application will fail sooner and provide an indication that the error is with injection rather than render().

[agubler]

I guess why would you register a an injector to load asynchronously?

Basically injectors should be set up so they are available when the dependent widget/container actually renders... having the warn would highlight that to a consumer. I can't think of a scenario where you'd want to have the actual state provider (store etc) load async (not saying there isn't one)

[devpaul]

I think trying to come up with a perfect situation where you'd need to load an injector asynchronously might be a blind alley. The purpose of the example provided was to illustrate when a console.warn wouldn't be adequate in development.

To restate, the benefits of calling getProperties() with undefined are:

1. it would fail faster and produce a more accurate stack trace
2. allow for default values to be provided by getProperties so Container could then fulfill its typings
3. it would allow for better monitoring of failures

If we console.warn none of the above are satisfied. If we throw an error we miss out on reacting to the error (outside of overriding Container internals to catch it).

Why would we not want to call getProperties with undefined?

[agubler]

Default properties should be provided by the actual external state or if the values aren’t available in the external state then they can be defaulted in getPropeties.

I am reluctant to allow undefined as the injected state because I don’t think it’s a friendly API and that consumers would have to guard against in all uses of getProperties (Which as I’ve mentioned should never be the case).

Surely the warn is better than a stack trace because it can tell the developer exactly what the issue is rather than them needing to deal with it themselves.

Something like: “Warn, injector ‘app-state’ has not been registered”

[agubler]

In addition you can’t define an injector that loads asyncronously (like you can a widget in the registry) so the injector not being registered really isn’t sometime that a consumer shouldn’t have to deal with it in their mapper.

Basically, injectors should be registered before they are required.

[devpaul]

I understand your reluctance. Injectors should be registered synchronously at the start of the application. That's the right way to do it.

I don’t think it’s a friendly API and that consumers would have to guard against in all uses of getProperties

The payload type for getProperties is any. Passing undefined wouldn't change the signature of getProperties or force the need for guards under standard usage.

That being said, an unregistered injector sounds like something that should never happen so I don't feel like pushing much harder on this issue is going to get us a lot of benefit. Maybe we can revisit this when we have a more concrete use case beyond catching and logging improper usage.

[agubler]

@devpaul I’ve overlooked the obvious reason why I didn’t warn when I initially implemented the functionality. It’s for Themed, we essentially register an “optional” injector that will only run if a consumer has added a theme to the registery under a known key.

Let’s continue to think on the best way forward...

[devpaul]

Playing devil's advocate, In the context of Themed if we wanted to enforce the rule that all Injectors should be registered before their use, we would:

• update @inject to call console.warn if an injector isn't registered.
• update Themed to register an Injector on module load with an empty payload (undefined?)
• update Themed's getProperties to do nothing with an empty payload

In the case where a theme hasn't been registered, getProperties would receive undefined (the empty payload) from the injector.

This is the same outcome as the other option of always calling getProperties and passing undefined if the Injector isn't present.

Frankly I think we should make the change to always call getProperties. It lowers the level of abstraction and coordination required and judging by Themed there will be cases in the future where there may be optional functionality that will want to do something when the injector is not defined.

Edit: After talking w/ Ant & Matt I realized that the module cannot register an injector without access to the registry, making the above steps unpossible.