fix(engine-core): use class name when invoking toString() on component #1963
Conversation
| @@ -44,6 +44,9 @@ export function registerComponent( | |||
| Ctor: ComponentConstructor, | |||
| { name, tmpl: template }: { name: string; tmpl: Template } | |||
There was a problem hiding this comment.
Based on the types, name can't be undefined. The type definition needs to be realigned.
| ): ComponentConstructor { | ||
| if (isUndefined(name)) { |
There was a problem hiding this comment.
any particular reason for the compiler to provide an undefined name?
There was a problem hiding this comment.
It has never been passed as far as I can tell:
lwc/packages/@lwc/babel-plugin-component/src/post-process/transform.js
Lines 117 to 120 in 20275fc
At best the compiler will be able to set the name as Ctor.name. IMO we should remove the optional name and just retrieve it from the Ctor.name.
There was a problem hiding this comment.
I agreed with that assessment because the compiler can't take it out of the Ctor, because it might be circular, and who knows what other quirk in platform. I'm ok with the runtime inference here.
| const elm = createElement('x-anonymous', { is: Anonymous }); | ||
| expect(elm.getToString()).toBe('[object BaseLightningElement]'); | ||
| expect(elm.getToString()).toBe('[object BaseLightningElementConstructor]'); |
There was a problem hiding this comment.
Not sure what to do here, the this value of a component is different in compat mode so String(this) returns _class.
| @@ -237,16 +230,7 @@ export function getComponentInternalDef(Ctor: unknown, name?: string): Component | |||
| ); | |||
| } | |||
|
|
|||
| let meta = getComponentRegisteredMeta(Ctor); | |||
| if (isUndefined(meta)) { | |||
| // TODO [#1295]: remove this workaround after refactoring tests | |||
There was a problem hiding this comment.
Thanks again, @ravijayaramappa!
| expect(elm.getToString()).toBe('[object BaseLightningElement]'); | ||
| }); | ||
| if (process.env.COMPAT !== true) { | ||
| it('should fallback to BaseLightningElementConstructor if constructor has no name', () => { |
There was a problem hiding this comment.
The constructor name ends up as _class in compat mode. This seems fine, given that it's no worse than the undefined that we render today.
There was a problem hiding this comment.
I'm ok with that. Who cares about compat :)?
| } | ||
|
|
||
| def = createComponentDef(Ctor, meta, name || Ctor.name); | ||
| def = createComponentDef(Ctor, name || Ctor.name); |
There was a problem hiding this comment.
why do we need the second argument? if we are going to do that in createComponentDef?
| @@ -96,11 +91,9 @@ function getCtorProto(Ctor: any, subclassComponentName: string): ComponentConstr | |||
|
|
|||
| function createComponentDef( | |||
| Ctor: ComponentConstructor, | |||
| meta: ComponentMeta, | |||
| subclassComponentName: string | |||
There was a problem hiding this comment.
what is the purpose of subclassComponentName at this point?
There was a problem hiding this comment.
Looks like we were just passing around so that it could be used in some error logs inside getCtorProto(). I went ahead and removed all that!
| @@ -68,11 +63,11 @@ export interface ComponentDef { | |||
|
|
|||
| const CtorToDefMap: WeakMap<any, ComponentDef> = new WeakMap(); | |||
|
|
|||
| function getCtorProto(Ctor: any, subclassComponentName: string): ComponentConstructor { | |||
| function getCtorProto(Ctor: any): ComponentConstructor { | |||
There was a problem hiding this comment.
| function getCtorProto(Ctor: any): ComponentConstructor { | |
| function getCtorProto(Ctor: ComponentConstructor): ComponentConstructor { |
| const superDef: ComponentDef = | ||
| (superProto as any) !== BaseLightningElement |
There was a problem hiding this comment.
| const superDef: ComponentDef = | |
| (superProto as any) !== BaseLightningElement | |
| const superDef = | |
| superProto !== BaseLightningElement |
| const superDef: ComponentDef = | ||
| (superProto as any) !== BaseLightningElement | ||
| ? getComponentInternalDef(superProto, subclassComponentName) | ||
| ? getComponentInternalDef(superProto) | ||
| : lightingElementDef; | ||
| const SuperBridge = isNull(superDef) ? BaseBridgeElement : superDef.bridge; |
There was a problem hiding this comment.
Based on the types, superDef can't be null here anymore.
| const SuperBridge = isNull(superDef) ? BaseBridgeElement : superDef.bridge; | |
| const SuperBridge = superDef.bridge; |
Details
Fixes #1033 by implementing the proposed fix in #1496.
Does this PR introduce breaking changes?
No, it does not introduce breaking changes.GUS work item
W-7111501