feat: better error message when event listener is missing #656
Conversation
| // TODO: Figure out where to catch the real thrown event | ||
| expect(() => { | ||
| element.triggerFoo(); | ||
| }).toThrow("Event listener for event 'foo' was not found."); |
There was a problem hiding this comment.
Does anyone have any suggestions for where to catch this exception?
Although the exception is thrown directly after triggering the event handler, something in the engine delays the exception from reaching the test until much later, leading to an uncaught exception.
| if (process.env.NODE_ENV !== 'production') { | ||
| throw new ReferenceError(`Event listener for event '${event.type}' was not found.`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this the right place for performing this check? I saw a few other potential places, but this seemed to align closest with existing behavior. The other options I saw were:
- The handler is generated when the engine loads. We're able to throw an error when we bind the handler itself at '$api_b'.
- The other suggestion was to provide a warning/error during compilation, but I couldn't find a good place where we knew both the binding on the template and the full list of functions on the component.
There was a problem hiding this comment.
this will swallow the error in production, which is not a good thing. I will propose this:
if (process.env.NODE_ENV !== 'production') {
assert.isTrue(isFunction(fn), `Event listener for event '${event.type}' was not found.`);
}
callHook(thisValue, fn, [event]);In this case, it doesn't introduce an extra if condition in prod, the check happens before the invocation.
Another problem here is that in order to see that error, the event must be dispatched. This itself makes is hard to give early errors. I think there are other places where this might be more useful, e.g.: api.b could do that assertion.
There was a problem hiding this comment.
api.b could do the assertion earlier, but we wouldn't be able to provide any meaningful context for the error if we do it there because the event and event type are only provided to api.b when the dispatch happens.
We could enhance api.b to have that context, and the compiler to provide it.
// component
on: {
"foo": api_bind(__getKey$3($cmp, "foo"), "foo")
}
// engine
export function b(fn: EventListener, fnName: string) {
...
if (process.env.NODE_ENV !== 'production') {
assert.isTrue(isFunction(fn), `Event listener '${fnName}' was not found.`);
}
...
}We can now reference the listener by name, but we're also passing an extra parameter for the sole purpose of a DEV mode error message.
There was a problem hiding this comment.
I can see two path forward here:
- Let the bind method to receive only the
fnName, and we can resolve the method based on the current vm being rendered.
export function b(fnName: string): EventListener {
if (isNull(vmBeingRendered)) {
throw new Error();
}
const vm: VM = vmBeingRendered;
const fn = vm.component[fnName];
if (process.env.NODE_ENV !== 'production') {
assert.isTrue(isFunction(fn), `Event listener '${fnName}' was not found on ${vm}.`);
}
return function(event: Event) {
if (vm.fallback) {
patchEvent(event);
}
invokeEventListener(vm, fn, vm.component, event);
};
}In any case, this is cached by the compiler, so, no much of a problem here.
- compiler does all the work, since it will cache those bindings, it could throw.
This is more complicated because throw is not allowed in expressions like the ternary operator that we use today for caching purposes.
I will favor 1, as it will tell you very clearly, what is the binding that you are using from the template that does not exist on the instance.
There was a problem hiding this comment.
If we're able to resolve the fnName from the VM, let's do that then. Like you said, it's clearer, and it's also more easily testable.
There was a problem hiding this comment.
Resolving the method from the method name works for listeners bound to the component, but isn't able to deal with listeners bound to some other data structure. The most common scenario is when when using an iterator.
// component
api_iterator(__getKey$3($cmp, "list"), function(item) {
...
on: {
// item is specific to this scope
"foo": api_bind(__getKey$3(item, "foo"))
}
...
})We could pass the scope for the binding down (api_bind("foo", item) or api_bind("foo", $cmp)), but that greatly increases the complexity of api.b, and doesn't seem much better than just resolving the function first then passing the function name down with it.
For the second path you mentioned, where does the compiler cache the bindings? I see where we generate the code to memoize the handlers when they're bound to the component instance, but not where we're cross referencing the bindings on the template with the component instance to cache the bindings themselves.
There was a problem hiding this comment.
jaja! bad bad bad memory! It is my fault, I designed the api.b a while ago, I should have remembered that limitation. <a onclick={foo.bar.baz}>.
There was a problem hiding this comment.
Let's go with the original proposal, it is a limited solution, but covers a good portion. Make sure that you look at my first comment on how to make it more performant.
Benchmark resultsBase commit: lwc-engine-benchmark-ie11
lwc-engine-benchmark
|
| @@ -125,7 +125,13 @@ export function invokeEventListener(vm: VM, fn: EventListener, thisValue: undefi | |||
| establishContext(context); | |||
| let error; | |||
| try { | |||
| callHook(thisValue, fn, [event]); | |||
| if (fn) { | |||
There was a problem hiding this comment.
should we check if 'thisValue' exists since its type can be undefined?
There was a problem hiding this comment.
It looks like that's a valid use case though. See https://github.com/salesforce/lwc/blob/master/packages/lwc-engine/src/framework/component.ts#L116
| callHook(thisValue, fn, [event]); | ||
| } else { | ||
| if (process.env.NODE_ENV !== 'production') { | ||
| throw new ReferenceError(`Event listener for event '${event.type}' was not found.`); |
There was a problem hiding this comment.
Should the message say 'handler' instead? I can see the wording being more intuitive since the devs won't manually attach listeners but specify an even handler in their template, which can make the error message more obvious.
There was a problem hiding this comment.
I went with 'listener' since that's the wording we used in our docs: http://lwc.sfdc.es/guide/events.html#Attach-a-Listener-Declaratively.
We do also use 'handler' in the same section of the doc though; do we have a consistent wording for 'listener' vs 'handler'?
Benchmark resultsBase commit: lwc-engine-benchmark-ie11
lwc-engine-benchmark
|
| @@ -125,6 +125,9 @@ export function invokeEventListener(vm: VM, fn: EventListener, thisValue: undefi | |||
| establishContext(context); | |||
| let error; | |||
| try { | |||
| if (process.env.NODE_ENV !== 'production') { | |||
| assert.isTrue(isFunction(fn), `Event listener for event '${event.type}' was not found.`); | |||
There was a problem hiding this comment.
Probably change this error to be more specific, something like: Invalid event handler for event 'foo' in ${vm} so they at least know what this is, and where it is happening.
Benchmark resultsBase commit: lwc-engine-benchmark-ie11
lwc-engine-benchmark
|
Details
Provide a better error message when event listener is missing
Addresses #325
Does this PR introduce a breaking change?
If yes, please describe the impact and migration path for existing applications:
Please check if your PR fulfills the following requirements: