🦋 Changeset detected

Latest commit: b3d4fb0

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

_{tachometer-reporter-action v2 for Benchmarks}

The size of lit-html.js and lit-core.min.js are as expected.

We are encountering an issue in the Chromium codebase as part of migrating to using the accessor keyword (to remove our dependency to useDefineForClassFields:false) along with reflect: true, which sounds that this new useDefault: true option might solve?

Please see https://issues.chromium.org/issues/389737066#comment12 for more context or directly see this minimal Lit playground repro

Note: This is an alternative to #4895, only one should be merged.

Completely ignoring the initial value would probably not work for us, as we are already getting failed tests when migrating to the accessor keyword due to this behavior.

Having said that, if this option indeed solves our issue (tried it on the minimal repro in the playground, but didn't, perhaps not rolled out yet?), we would still need to add useDefault: true everywhere where we use reflect: true which sounds quite cumbersome.

@freshp86 can you open an issue?

It seems like the issues is that with experimental decorators and auto-accessors, the initial value isn't reflected? I'll have to check our tests for that case.

@sorvell on the linked playground, adding wrapped: true fixes the issue. Maybe this is a case of decorating the getter vs the setter.

can you open an issue?

No issue is needed. This change corrects this behavior.

@freshp86 This PR should also fix your issue, regardless of using useDefault. We were only properly handling accessor when using the @property decorator, not static properties. This was corrected here via this PR.

As @justinfagnani mentioned, while waiting for this release, you can add wrapped: true to the property options to correct the behavior. This workaround will be unnecessary in the next Lit release, but it is compatible with it (repro with workaround)

As @justinfagnani mentioned, while waiting for this release, you can add wrapped: true to the property options to correct the behavior.

Thanks for the quick response! I verified that adding wrapped: true fixes both the Lit playground repro, as well as one of the tests that were failing in Chromium (will verify remaining tests shortly). I don't see the wrapped option documented at https://lit.dev/docs/components/properties/#property-options though, so curious if this is considered part of the public API.

This workaround will be unnecessary in the next Lit release, but it is compatible with it (repro with workaround)

Do you have any estimate on when the next release will come out? In Chromium we still use v3.0.2 here, which is already behind latest 3.2.1, so we'll need to roll forward a few versions to get this fix.

In the meantime, do you think locally patching these lines that you referernced in our Chromium copy of Lit would be sufficient to fix the issue, without us having to update to later releases (and without having to add wrapped: true)?

In the meantime

I think it's probably simpler to use the undocumented wrapped: true rather than patching, and just update when we publish the next version. We can probably do a release this week.

Might have found an even easier fix for us, which does not require updating client code with wrapped: true, by automatically adding it during our static override finalize() {...} method for the CrLitElement class (which all other Lit element's extend from). See https://chromium-review.googlesource.com/c/chromium/src/+/6349561. I have verified this fixes previously failing tests, but still waiting to confirm that no other tests break because of it.

Might have found an even easier fix for us, which does not require updating client code with wrapped: true

If you have a base class, probably the simplest way would be:

static createProperty(name, options) {
  super.createProperty(name, {...options, wrapped: true});
}

If you have a base class, probably the simplest way would be:

Don't we need to check for if (this.prototype.hasOwnProperty(name)) {...} similar to your PR to only add the wrapped: true for such cases? In the superclass we still need to deal with both cases (using/not-using accessor). Also, don't we only need to add this for cases where reflect: true instead of adding it to all reactive porperties?

Don't we need to check for if (this.prototype.hasOwnProperty(name)) {...}

It's only strictly needed there, but you don't need to discriminate it. Setting wrapped forces the property to be in the changed properties if it has a value on first update. Valued non-own properties are already in changed properties.

Also, don't we only need to add this for cases where reflect: true instead of adding it to all reactive porperties?

No, it should not be based on the value of reflect. As noted, setting wrapped ensures the property is in the changed properties. This makes reflection work and also ensures the property is included in changed properties (e.g. updated(changed /* in this map */) )

Thanks for the info! I will update my CL and tests at https://chromium-review.googlesource.com/c/chromium/src/+/6349561 and I might add you as a reviewer as well if you don't mind. Thanks!

As noted, setting wrapped ensures the property is in the changed properties...

So, while writing tests to test changed properties as well, discovered that there is still a difference in behavior between non-accessor and acessor properties on first load. Minimal repro here. In short accessor properties while indeed present in changed properties when wrapped: true is used, their "previous" value reported is not undefined, which IIUC it should, see snippet below. (Perhaps this is also fixed as part of this PR?).

willUpdate(changedProperties: PropertyValues<this>) {
  super.willUpdate(changedProperties);
  // expecting 'undefined', but instead initial value is reported as the "previous" value.
  console.log(changedProperties.get('someAccessorProperty')); 
}

In short accessor properties while indeed present in changed properties when wrapped: true is used, their "previous" value reported is not undefined

Yeah, I should have mentioned that. This is also fixed in this change, and you can patch it in if you need, but it's also technically safe to do in your base class like this (since _$* properties are not renamed):

fixed repro with code below

override protected performUpdate() {
  if (!this.isUpdatePending) {
    return;
  }
  // this loop is duplicated in the super call but the work won't occur twice due to the 
  // `_$changedProperties` check
  if (!this.hasUpdated) {
    const {elementProperties} = (this.constructor as typeof ReactiveElement);
    for (const [p, options] of elementProperties) {
      if (options.wrapped === true && !this._$changedProperties.has(p) &&
          this[p as keyof this] !== undefined) {
        this._$changeProperty(p, undefined, options);
      }
    }
  }
  super.performUpdate();
}

Thanks for the detailed response, this is indeed very helpful and does fix the issue!

Having said that, fixing the issue in our CrLitElement base is a bit complicated. Specifically, after verifying that the above works in the minimal repro, plugging it in CrLitELemnet didn't initially work, and took quite a bit to debug. The problem is that we operate on the minified version of ReactiveElement from node_modules/@lit/reactive-element/reactive-element.js, where the internal APIs _$changedProperties and _$changeProperty are minified to _$AL and C respectively (took a while to figure out why _$changedProperties was undefined). We also need to make TypeScript happy while using those internal APIs, so the end result looks as follows:

  override performUpdate() {
    if (!this.isUpdatePending) {
      return;
    }
    if (!this.hasUpdated) {
      interface WithInternalApis extends CrLitElement {
        // minified name for $changedProperties
        _$AL: PropertyValues;
        // minified name for _$changeProperty()
        C(p: PropertyKey, v: any, options: any): void;
      }

      const {elementProperties} = this.constructor as typeof ReactiveElement;
      for (const [p, options] of elementProperties) {
        if ((options as {wrapped?: boolean}).wrapped === true &&
            !(this as unknown as WithInternalApis)._$AL.has(p) &&
            this[p as keyof this] !== undefined) {
          (this as unknown as WithInternalApis).C(p, undefined, options);
        }
      }
    }
    super.performUpdate();
  }

I think this qualifies as a hack :> You can see the complete change at https://chromium-review.googlesource.com/c/chromium/src/+/6349561/7/third_party/lit/v3_0/cr_lit_element.ts.

So, the sooner we can get the official fix the sooner we can remove this hack. Rolling our Lit version to the latest Lit version is also blocked by #4943, so the current plan is

1. Land the hacky fixes directly in CrLitElement, to unblock the migration to the accessor keyword (https://issues.chromium.org/issues/389737066).
2. Roll our Lit version to 3.2.1 as soon as there is a fix for [ts5.8.2+node16] error TS2307: Cannot find module 'trusted-types/lib' or its corresponding type declarations #4943 (we would like to avoid adding skipLibCheck across the entire repo), https://issues.chromium.org/issues/402800778.
3. Roll to 3.2.2 (or whichever version will have these fixes) and remove the workarounds in CrLitElement.

Thanks again!

@sorvell Unfortunately discovered one more difference in changed properties behavior between wrapped and non-wrapped properties, when a value is passed from the parent, see new minimal repro here, where the 'previous' value of the 1st willUpdate() and updated() callbacks is not undefined (which was revealed by some of our Chromium tests failing when we deploy the 'accessor' keyword + our workaround more widely, which are failing this assertion).

Note that the problem happens even if the && !this._$changedProperties.has(p) part is removed from the following condition

if (options.wrapped === true && !this._$changedProperties.has(p) && this[p] !== undefined) {    
  this._$changeProperty(p, undefined, options);
}

and the this._$changeProperty(...) call seems to have no effect basically.

Do you know if the fix in this PR addresses this, or if this is just a problem with the temporary workaround we discussed in earlier comments. Is there a way to make these cases behave identically across wrapped and non-wrapped properties?

Update: Possible refinement to our workaround is to directly call this._$changeProperties.set() when the property is already a member, instead of calling this._$changeProperty(...) , which seems to ignore the oldValue passed to it in such cases in this line, see possible CrLitElement diff here, not sure if this is 100% safe to do because it skips the part in _$changeProperty() that updates __reflectingProperties, but it does seem to fix the issue locally.

Perhaps calling both as follows is necessary?

        this._$changeProperty(p, undefined, options);
        this._$changedProperties.set(p, undefined);

Ultimately I think we need a way to skip the if check at this line https://github.com/lit/lit/blob/main/packages/reactive-element/src/reactive-element.ts#L1338.

Do you know if the fix in this PR addresses this

No, see #4916 where I linked your comment. Maybe best to continue, if needed, there.