fix: fix an issue in the custom elements bundle where if you set a property on an element programatically before it was upgraded it would shadow the accessors and therefore break any watchers as well. See https://developers.google.com/web/fundamentals/web-components/best-practices\#lazy-properties

[nphyatt]

This fixes an issue in the custom elements bundle where if you set a property on an element before it is upgraded it will forever shadow the accessors and therefore Watchers will no longer fire.

This is related to and potentially fixes #2456

The approach here is to check if the element hasOwnProperty in the connectedCallback and unshadow that property and restore the value that was shadowing the accessors by using the setter to reset the value. This is the best practice described here: https://developers.google.com/web/fundamentals/web-components/best-practices#lazy-properties

We were already doing this but only for the lazy loaded bundle. This PR does it for both.

I also had to add logic to the attributeChangedCallback because if fires before the connectedCallback event and we set the property there so if it's shadowed we end up setting the shadowed property with the original stale value that was set inline. I believe we should thing about moving the logic to set the property outside of attributeChangedCallback as is the best practice described here: https://developers.google.com/web/fundamentals/web-components/best-practices#avoid-reentrancy however that seemed like a bit of a can of worms and outside the scope of this fix.

I've only tested this fix against the custom elements build so I would like to double check it against the lazy build before we merge.

[nphyatt]

I was able to manually test on the lazy bundle and confirm this does fix the issue there as well and works as intended manually. I had thoughts of trying to write some tests around the behavior but it was not immediately clear to me if one could / how one would go about writing a test in which they attempt change a property on a component in the dom before it has been upgraded.

[rwaskiewicz]

One minor comment on the JSDoc

[rwaskiewicz]

I took a stab at reworking this comment block a section a bit, LMK what you think (especially if I didn't capture the original messaging correctly 🙃)

Suggested change

	// the attr changed callback runs prior to the connected callback where we have logic that also
	// has similar logic in it to support lazy properties so if we attempt to unshadow in both places
	// to cover the case where an attr was set inline on the non-upgrade element and then the property
	// was programatically set which will be handled here or the case where the attr was not set in
	// which case the connectedCallback will catch and unshadow.
	// https://developers.google.com/web/fundamentals/web-components/best-practices#lazy-properties
	// we should think about whether or not we actually want to be reflecting the attributes to properties
	// here given that this goes against best practices outlined here
	// https://developers.google.com/web/fundamentals/web-components/best-practices#avoid-reentrancy
	// The attributeChangedCallback may run prior to Stencil's connectedCallback, which may also attempt to
	// unshadow lazy properties. The case where an attribute was set inline on the non-upgraded element:
	// ```html
	// <!-- this component has _not_ been upgraded yet -->
	// <my-component some-attribute="some-value></my-component>
	// ```
	// and the property was subsequently set programmatically shall be handled here.
	//
	// The case where the attribute was set inline but was not set programmatically shall be handled/unshadowed
	// by connectedCallback.
	//
	// https://developers.google.com/web/fundamentals/web-components/best-practices#lazy-properties
	//
	// TODO(STENCIL-16) we should think about whether or not we actually want to be reflecting the attributes to
	// properties here given that this goes against best practices outlined here
	// https://developers.google.com/web/fundamentals/web-components/best-practices#avoid-reentrancy

[rwaskiewicz]

Tested manually with https://github.com/kensodemann/demo-stencil-watch/commit/da64cc780a8f7cd3c40c0cdd78342372d34d1392 for Custom Elements Build and Lazy Loading. For both of these, I checked out this branch, rebased it off the HEAD of master (since the branch doesn't include the fix for running npm i on an M1), then ran npm ci && npm build && npm pack

In the demo-stencil-watch repo's directory, I installed base deps, then the tarball from my local filesystem and built:
npm ci && npm i <PATH_TO_TARBALL> && npm run build

Using http-server I could then serve the custom element bundle:

http-server -c0 site

or the lazy bundle:

http-server -c0 www

For the custom elements bundle and the lazy loaded bundle, it's important to alter the index.html file in site and src, respectively.

The easiest way to test is to uncomment the invocation of runTest() and comment out the customElements.whenDefined call:

-    // runTest();
-    customElements.whenDefined('my-component').then(runTest);    
+    runTest();
+    // customElements.whenDefined('my-component').then(runTest);

[rwaskiewicz]

Suggested change

	// unshadow lazy properties. If we attempt to unshadow in both places
	// unshadow lazy properties. If we attempt to unshadow in both places

[rwaskiewicz]

Can we add the TODO that I originally proposed here? I'd like to start tracking things like this in our planning tools (I may have created a placeholder ticket 😉).

Suggested change

	// we should think about whether or not we actually want to be reflecting the attributes to properties
	// TODO(STENCIL-16) we should think about whether or not we actually want to be reflecting the attributes to properties

Or did you want to have that conversation as a part of this ticket?