feat(engine): improve createElement() second parameter error messages #944
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Benchmark resultsBase commit: lwc-engine-benchmark
|
caridy
reviewed
Jan 11, 2019
| if (!isObject(options) || isNull(options)) { | ||
| throw new TypeError(); | ||
| throw new TypeError(`"createElement" function was invoked with invalid second parameter "${options}". Expected an object that contains property "is" mapped to an object that extends LightningElement from "lwc".`); |
There was a problem hiding this comment.
you have to call ${toString(options)} in case the argument can't be transformed into a string.
caridy
reviewed
Jan 11, 2019
|
|
||
| if (isNull(Ctor) || isUndefined(Ctor)) { | ||
| throw new TypeError( | ||
| `"createElement" function was invoked with invalid second parameter. "is" property value was "${Ctor}", but expected an object that extends LightningElement from "lwc". ` + |
pmdartus
requested changes
Jan 11, 2019
| if (!isObject(options) || isNull(options)) { | ||
| throw new TypeError(); | ||
| throw new TypeError(`"createElement" function was invoked with invalid second parameter "${options}". Expected an object that contains property "is" mapped to an object that extends LightningElement from "lwc".`); |
There was a problem hiding this comment.
We should keep the first error message minimal and focussed. Something like sound be enough:
"createElement" expects an object as second parameter.
If we were to add another required parameters to createElement we would need to update both error messages with the error messages you proposed.
|
|
||
| if (isNull(Ctor) || isUndefined(Ctor)) { | ||
| throw new TypeError( | ||
| `"createElement" function was invoked with invalid second parameter. "is" property value was "${Ctor}", but expected an object that extends LightningElement from "lwc". ` + |
There was a problem hiding this comment.
The error is inaccurate here: You probably forgot to add the extend clause on the class declaration.. We are only checking if the is property is not null or undefined.
Since we are validating if the is value in getComponentDef, we should only warn if the Ctor is from the wrong type.
| } | ||
|
|
||
| let Ctor = (options as any).is as ComponentConstructor; | ||
|
|
||
| if (isNull(Ctor) || isUndefined(Ctor)) { |
There was a problem hiding this comment.
Instead of checking if the function is null or undefined, I would rather check if the Ctor is a function.
Benchmark resultsBase commit: lwc-engine-benchmark
|
caridy
approved these changes
Jan 11, 2019
pmdartus
approved these changes
Jan 14, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Improve createElement second parameter validation. Fixes #940
Does this PR introduce a breaking change?