fix: use more helpful error message for invalid render() return value @W-14785069

[wjhsf]

Details

Pretty much what it says on the tin. I just reused a pre-existing error message:

lwc/packages/@lwc/engine-core/src/framework/template.ts

Lines 250 to 252 in a1831c1

	`Invalid template returned by the render() method on ${vm}. It must return an imported template (e.g.: \`import html from "./${
	vm.def.name
	}.html"\`), instead, it has returned: ${toString(html)}.`

Fixes #3924.

Does this pull request introduce a breaking change?

• ✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

• ⚠️ Yes, it does include an observable change.

GUS work item

W-14785069

[nolanlawson]

I think this error message can be more effective if we target the root cause of the error.

I would suggest adding a logError call to the invokeComponentRenderMethod function here:

lwc/packages/@lwc/engine-core/src/framework/invoker.ts

Line 110 in 4d4d768

return renderInvocationSuccessful ? evaluateTemplate(vm, html!) : [];

At this point in the code, we know if html is undefined (or some other invalid value). We can provide an error at the soonest possible moment, which would help a component author trying to use breakpoints in DevTools to debug.

BTW the reason I recommend logError rather than throwing an Error is that there may be some cases where a component author is calling render() but would not reach the assert.isTrue in template.ts. E.g. they may be invoking render() themselves manually. (It sounds strange, but we've seen component authors do stranger stuff.) So if we threw an Error, we would produce a new failure condition that a component might not have hit before (i.e. a breaking change).

The reason I recommend logError rather than logWarn is that the yellow text may get lost in a sea of other warnings, whereas the red text would stand out, and would be printed before the red text for the template.ts assert.isTrue message. (This error message doesn't necessarily need to change; it is still accurately reporting the error it's hitting.)

[wjhsf]

I think this error message can be more effective if we target the root cause of the error.

I would suggest adding a logError call to the invokeComponentRenderMethod function here:

lwc/packages/@lwc/engine-core/src/framework/invoker.ts

Line 110 in 4d4d768

return renderInvocationSuccessful ? evaluateTemplate(vm, html!) : [];

At this point in the code, we know if html is undefined (or some other invalid value). We can provide an error at the soonest possible moment, which would help a component author trying to use breakpoints in DevTools to debug.

BTW the reason I recommend logError rather than throwing an Error is that there may be some cases where a component author is calling render() but would not reach the assert.isTrue in template.ts. E.g. they may be invoking render() themselves manually. (It sounds strange, but we've seen component authors do stranger stuff.) So if we threw an Error, we would produce a new failure condition that a component might not have hit before (i.e. a breaking change).

I'm a bit confused by this. Your suggestion is to add the error to invokeComponentRenderMethod, which ends with a call to evaluateTemplate, which has the assertion as the first statement. So you can't currently call invokeComponentRenderMethod without also calling evaluateTemplate and reaching the assertion (unless render() throws). So your comment that someone could call render() without reaching the assertion implies that calling render doesn't call invokeComponentRenderMethod. But in that case, why would it matter what we do in invokeComponentRenderMethod? (Unless you're talking about the case where render() throws an error, which doesn't seem to be the case, because that's a different sort of error.)

In any case, the conditional check already exists in evaluateTemplate and invokeComponentRenderMethod always calls evaluateTemplate, so I think that it makes sense to log the additional message in the same spot as the thrown error, rather than in a different function. Plus, more abstractly, I think that it makes more sense for the "this is a bad template" error to come from the thing called "evaluate template", rather than "invoke method". And yeah, technically that means we're losing out on the stack trace, but I don't think a single extra function in the stack is super consequential.

Additionally, I think that a single error ("bad template") would ideally have a single error message, which should contain sufficient context to understand the error. Splitting the context across messages feels suboptimal, but if we want to keep the original thrown error message for backwards compatibility, then I guess multiple messages is really the only way to go.

[nolanlawson]

@wjhsf You're right, I just double-checked the code; I didn't realize that invokeComponentRenderMethod directly invokes evaluateTemplate. For some reason I thought evaluateTemplate happened in an additional microtask.

In that case no worries, let's go with your suggestion of improving the error message. Could you please also add a Karma test to confirm that the error is thrown in dev mode? We have a custom Jasmine matcher called toThrowErrorDev that can help with this.

[nolanlawson]

Suggested change

Error,

You don't need to assert the Error class if you're just checking for a generic Error.

[wjhsf]

lwc/packages/@lwc/integration-karma/README.md

Line 82 in 175d7aa

- `toThrowErrorDev(Error, message)`: `expect` a function to throw an error with a specific Error constructor and a specific message.

Should update this, then, at some point...

[nolanlawson]

Oops yeah, that's wrong. The message can be a regex or a string as well.

[nolanlawson]

Ak, I just realized you will need toThrowCallbackReactionErrorDev because of native lifecycle. You can see why detailed here (scroll to "New error behavior").

[nolanlawson]

LGTM, one small nit

[nolanlawson]

⚠️ Yes, it does include an observable change.

FWIW, we mostly use "observable change" to mean "something that might break a customer in an inadvertent way." This one doesn't really pass that small test for me, since a customer would have to 1) catch the error and assert something about the error message, and 2) only in dev mode.

We should probably change the template to just say "breaking/observable change," since that's what we really care about.

[nolanlawson]

The tests are failing:

	Expected function to throw an Error error in development mode "with message /Invalid template returned by the render\(\) method of x-render-invalid/", but it threw TypeError with message "undefined is not an object (evaluating 'template.renderMode')"