fix(engine-core): use class name when invoking toString() on component #1963

ekashida · 2020-07-06T05:41:45Z

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

pmdartus · 2020-07-06T07:48:00Z

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

@@ -44,6 +44,9 @@ export function registerComponent(
    Ctor: ComponentConstructor,
    { name, tmpl: template }: { name: string; tmpl: Template }


Based on the types, name can't be undefined. The type definition needs to be realigned.

caridy · 2020-07-06T14:22:01Z

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

 ): ComponentConstructor {
+    if (isUndefined(name)) {


any particular reason for the compiler to provide an undefined name?

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

return t.callExpression(id, [

node,

t.objectExpression([t.objectProperty(t.identifier('tmpl'), templateIdentifier)]),

]);

.

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.

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.

Outdated based on Caridy's comment.

ekashida · 2020-07-07T04:20:52Z

packages/integration-karma/test/component/LightningElement.toString/index.spec.js

    const elm = createElement('x-anonymous', { is: Anonymous });
-    expect(elm.getToString()).toBe('[object BaseLightningElement]');
+    expect(elm.getToString()).toBe('[object BaseLightningElementConstructor]');


Not sure what to do here, the this value of a component is different in compat mode so String(this) returns _class.

ekashida · 2020-07-07T05:29:31Z

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

@@ -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


Thanks again, @ravijayaramappa!

ekashida · 2020-07-07T21:08:40Z

@pmdartus @caridy ready for another pass

ekashida · 2020-07-07T21:12:07Z

packages/integration-karma/test/component/LightningElement.toString/index.spec.js

-    expect(elm.getToString()).toBe('[object BaseLightningElement]');
-});
+if (process.env.COMPAT !== true) {
+    it('should fallback to BaseLightningElementConstructor if constructor has no name', () => {


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.

I'm ok with that. Who cares about compat :)?

caridy · 2020-07-08T02:55:57Z

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

-        }
-
-        def = createComponentDef(Ctor, meta, name || Ctor.name);
+        def = createComponentDef(Ctor, name || Ctor.name);


why do we need the second argument? if we are going to do that in createComponentDef?

caridy · 2020-07-08T02:56:11Z

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

@@ -96,11 +91,9 @@ function getCtorProto(Ctor: any, subclassComponentName: string): ComponentConstr

 function createComponentDef(
    Ctor: ComponentConstructor,
-    meta: ComponentMeta,
    subclassComponentName: string


what is the purpose of subclassComponentName at this point?

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!

caridy

few minor questions... looking good.

pmdartus

I have some tiny type changes. Otherwise, the code looks great.

pmdartus · 2020-07-08T08:23:00Z

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

@@ -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 {


Suggested change

function getCtorProto(Ctor: any): ComponentConstructor {

function getCtorProto(Ctor: ComponentConstructor): ComponentConstructor {

pmdartus · 2020-07-08T08:24:44Z

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

    const superDef: ComponentDef =
        (superProto as any) !== BaseLightningElement


Suggested change

const superDef: ComponentDef =

(superProto as any) !== BaseLightningElement

const superDef =

superProto !== BaseLightningElement

pmdartus · 2020-07-08T08:31:57Z

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

    const superDef: ComponentDef =
        (superProto as any) !== BaseLightningElement
-            ? getComponentInternalDef(superProto, subclassComponentName)
+            ? getComponentInternalDef(superProto)
            : lightingElementDef;
    const SuperBridge = isNull(superDef) ? BaseBridgeElement : superDef.bridge;


Based on the types, superDef can't be null here anymore.

Suggested change

const SuperBridge = isNull(superDef) ? BaseBridgeElement : superDef.bridge;

const SuperBridge = superDef.bridge;

caridy

LGTM

fix(engine-core): use class name when invoking toString() on component

f4663df

ekashida added the next-release label Jul 6, 2020

pmdartus requested changes Jul 6, 2020

View reviewed changes

chore: review feedback

7ea3ed4

pmdartus previously approved these changes Jul 6, 2020

View reviewed changes

caridy reviewed Jul 6, 2020

View reviewed changes

ekashida added 2 commits July 6, 2020 18:21

refactor: read Ctor.name at def creation time

1af07b0

test: fix failing integration test

f9818fd

ekashida commented Jul 7, 2020

View reviewed changes

test: disable in compat mode

9472db9

ekashida commented Jul 7, 2020

View reviewed changes

caridy reviewed Jul 8, 2020

View reviewed changes

refactor: stop relaying the name from method to method

effbcfa

pmdartus approved these changes Jul 8, 2020

View reviewed changes

caridy approved these changes Jul 8, 2020

View reviewed changes

chore: review feedback

60575ad

ekashida merged commit e94ec15 into master Jul 9, 2020

ekashida deleted the ekashida/fix-to-string branch July 9, 2020 01:12

		@@ -44,6 +44,9 @@ export function registerComponent(
		Ctor: ComponentConstructor,
		{ name, tmpl: template }: { name: string; tmpl: Template }

	return t.callExpression(id, [
	node,
	t.objectExpression([t.objectProperty(t.identifier('tmpl'), templateIdentifier)]),
	]);

	function getCtorProto(Ctor: any): ComponentConstructor {
	function getCtorProto(Ctor: ComponentConstructor): ComponentConstructor {

		const superDef: ComponentDef =
		(superProto as any) !== BaseLightningElement

	const SuperBridge = isNull(superDef) ? BaseBridgeElement : superDef.bridge;
	const SuperBridge = superDef.bridge;

fix(engine-core): use class name when invoking toString() on component #1963

fix(engine-core): use class name when invoking toString() on component #1963

Uh oh!

Conversation

ekashida commented Jul 6, 2020

Details

Does this PR introduce breaking changes?

GUS work item

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekashida commented Jul 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caridy left a comment

Choose a reason for hiding this comment

Uh oh!

pmdartus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caridy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!