fix(engine-dom): do not leak pivot constructor #3112

nolanlawson · 2022-10-13T21:27:50Z

Details

Hides the pivot constructor as best we can, when running in ENABLE_SCOPED_CUSTOM_ELEMENT_REGISTRY mode. This better prevents users from "detecting" that the pivot patches are applied.

We can't fully fix this (see details in #3073), so this is a "best effort."

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.

With this change, customElements.get/customElement.whenDefined changes its behavior depending on whether or not a pivot (LWC) constructor was defined. But this only occurs if the flag is set, and we haven't enabled the flag anywhere yet.

GUS work item

W-11884701

nolanlawson · 2022-10-13T21:28:15Z

packages/@lwc/engine-dom/src/custom-elements/create-scoped-registry.ts

+    adoptedCallback?: () => void;
+    attributeChangedCallback?: (name: string, oldValue: any, newValue: any) => void;
+    observedAttributes: Set<string>;
+}


I just decided to move this up to have all the TypeScript types/interfaces together.

nolanlawson · 2022-10-13T21:28:54Z

packages/@lwc/engine-dom/src/custom-elements/create-scoped-registry.ts

    const globalDefinitionsByTag = new Map<string, Definition>();
    const globalDefinitionsByClass = new Map<CustomElementConstructor, Definition>();
    const awaitingUpgrade = new Map<string, Set<HTMLElement>>();
+    const pendingWhenDefinedCallbacks = new Map<string, WhenDefinedCallback[]>();


I considered keeping track of the Promise as well as the resolve function, so that we didn't need an array of callbacks and could just keep returning the same Promise, but it got really hairy to deal with the Promise constructor callbacks. An array was a lot simpler.

nolanlawson · 2022-10-13T21:30:07Z

packages/@lwc/integration-karma/test/custom-elements/index.spec.js

+                        expect(Ctor).not.toBeUndefined();
+                        expect(Ctor).toBe(firstCtor);
+                    });
+            });


These tests may seem excessive, but it's to get 100% code coverage.

ravijayaramappa · 2022-10-17T19:02:36Z

packages/@lwc/engine-dom/src/custom-elements/create-scoped-registry.ts

            }
        }
+        // If anyone called customElements.whenDefined() and is still waiting for a promise resolution, resolve now
+        flushPendingWhenDefinedCallbacks(tagName, constructor);


Question:
If the user register's a whenDefined callback for 'x-foo' and it is the engine registers its own constructor for x-foo. The user's callback will still be invoked in this flush operation, correct? Is that the desired operation? This would lead to inconsistency in behavior of the APIs.

// user registers callback for x-foo // engine defines x-foo with a pivot ctor unrelated to user // user's callback is flushed // Now user thin's the element is defined, runs customElements.get('x-foo') and I believe an `undefined` is returned

The meta question is, should the engine's usage of the native registry to perform pivoting be transparent to the user?

nvm, this case is handled in L492

I can add a test for the negative case, to confirm that the promise never resolves.

Nevermind, there is a test for that here: https://github.com/salesforce/lwc/pull/3112/files#r997496983

ravijayaramappa · 2022-10-17T19:19:39Z

packages/@lwc/integration-karma/test/custom-elements/index.spec.js

+                const promise = customElements.whenDefined(tagName).then((ctor) => {
+                    resolvedCtor = ctor;
+                });
+                expect(customElements.get(tagName)).toBeUndefined();


Between L66 and L71, the customElements registry is untouched except for registering the callback. In which case is it expected that customElements.get(tagName) will behave differently at L71?

No you're right, this line is unnecessary.

ravijayaramappa · 2022-10-17T19:23:44Z

packages/@lwc/integration-karma/test/custom-elements/index.spec.js

+                return Promise.resolve()
+                    .then(() => {
+                        expect(resolvedCtor).toEqual(undefined); // whenDefined does not leak pivot constructor
+                        customElements.define(tagName, MyCustomComponent);
+                        return promise;


Suggested change

return Promise.resolve()

.then(() => {

expect(resolvedCtor).toEqual(undefined); // whenDefined does not leak pivot constructor

customElements.define(tagName, MyCustomComponent);

return promise;

expect(resolvedCtor).toEqual(undefined); // whenDefined does not leak pivot constructor

customElements.define(tagName, MyCustomComponent);

return promise.

See https://github.com/salesforce/lwc/pull/3112/files#r997496983.

I'll add a comment about why Promise.resolve() is in there.

packages/@lwc/integration-karma/test/custom-elements/index.spec.js

ravijayaramappa · 2022-10-17T19:28:34Z

packages/@lwc/integration-karma/test/custom-elements/index.spec.js

-                    expect(Ctor).not.toBeUndefined();
-                    expect(Ctor).toBe(firstCtor);
-                });
+            it('whenDefined() should always return the same constructor - defined before whenDefined', () => {


Suggested change

it('whenDefined() should always return the same constructor - defined before whenDefined', () => {

it('whenDefined() should always return the same constructor - pivot defined before whenDefined', () => {

This test actually runs with and without pivots.

ravijayaramappa · 2022-10-17T19:28:57Z

packages/@lwc/integration-karma/test/custom-elements/index.spec.js

+                    });
+            });
+
+            it('whenDefined() should always return the same constructor - defined after whenDefined', () => {


Same as above - additional comments

See above; I'll add a comment

ravijayaramappa

LGTM, just some questions and minor comments

nolanlawson · 2022-10-17T20:48:27Z

packages/@lwc/integration-karma/test/custom-elements/index.spec.js

+                class MyCustomComponent extends HTMLElement {}
+                return Promise.resolve()
+                    .then(() => {
+                        expect(resolvedCtor).toEqual(undefined); // whenDefined does not leak pivot constructor


This ^ verifies that whenDefined() does not leak a private element created by LWC. (At least insofar as we assume Promise.resolve() would have caught it)

…ex.spec.js Co-authored-by: Ravi Jayaramappa <ravi.jayaramappa@salesforce.com>

…s-pivots-undefined-2

nolanlawson · 2022-10-19T18:36:11Z

/nucleus test

nolanlawson · 2022-10-19T21:20:32Z

/nucleus test

nolanlawson · 2022-10-19T21:43:26Z

/nucleus test

nolanlawson · 2022-10-19T21:43:44Z

/nucleus test

fix(engine-dom): do not leak pivot constructor

5d78a75

nolanlawson requested review from caridy and ravijayaramappa October 13, 2022 21:27

nolanlawson commented Oct 13, 2022

View reviewed changes

ravijayaramappa reviewed Oct 17, 2022

View reviewed changes

packages/@lwc/integration-karma/test/custom-elements/index.spec.js Show resolved Hide resolved

ravijayaramappa reviewed Oct 17, 2022

View reviewed changes

ravijayaramappa approved these changes Oct 17, 2022

View reviewed changes

nolanlawson commented Oct 17, 2022

View reviewed changes

nolanlawson and others added 4 commits October 17, 2022 13:55

test: remove unnecessary test line, add comment

38a3132

test: update packages/@lwc/integration-karma/test/custom-elements/ind…

81fe44c

…ex.spec.js Co-authored-by: Ravi Jayaramappa <ravi.jayaramappa@salesforce.com>

test: add comment

3bfb959

Merge remote-tracking branch 'origin/master' into nolan/node-reaction…

8d245cf

…s-pivots-undefined-2

nolanlawson merged commit 0635524 into master Oct 19, 2022

nolanlawson deleted the nolan/node-reactions-pivots-undefined-2 branch October 19, 2022 21:46

	it('whenDefined() should always return the same constructor - defined before whenDefined', () => {
	it('whenDefined() should always return the same constructor - pivot defined before whenDefined', () => {

fix(engine-dom): do not leak pivot constructor #3112

fix(engine-dom): do not leak pivot constructor #3112

Uh oh!

Conversation

nolanlawson commented Oct 13, 2022

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

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!

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!

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!

ravijayaramappa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nolanlawson commented Oct 19, 2022

Uh oh!

nolanlawson commented Oct 19, 2022

Uh oh!

nolanlawson commented Oct 19, 2022

Uh oh!

nolanlawson commented Oct 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants