[reactive-element] Adds skipping initial attribute reflection for default values. by sorvell · Pull Request #4895 · lit/lit

sorvell · 2025-01-17T00:25:12Z

Adds support for lit/rfcs#36 and helps facilitate shoelace-style/shoelace#2339.

Spawning an initial attribute can cause a NextJS hydration error, see repro.

To use, set defaultValue in property options, and avoid setting the default in the field/accessor.

@property({reflect: true, defaultValue: 'foo'})
variant: string;

Previously, this would spawn a variant="foo" attribute. With this PR and the above settings, it will not.

<my-element></my-element>

In addition, the default value will be set when the attribute is removed.

changeset-bot · 2025-01-17T00:25:15Z

🦋 Changeset detected

Latest commit: 3dfc1ce

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

This PR includes changesets to release 3 packages

Name	Type
@lit/reactive-element	Minor
lit	Minor
lit-element	Minor

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

justinfagnani · 2025-01-21T00:57:17Z

Nice!

My main comment before looking at code is that I don't think we should have the option skipReflectInitial. Attributes should ideally never reflect initially. If you give a default value, I see no benefit to reflecting it, even for styling.

If for some reason in the future enabling initial value reflection is important, we could add an reflectInitial option, but I wouldn't include it now.

I also think the option name should be defaultValue. value sounds too much like the value can't change. "default" implies better to me that that value will be used when the attribute is removed.

packages/reactive-element/src/reactive-element.ts

justinfagnani · 2025-02-03T20:16:31Z

packages/reactive-element/src/decorators/property.ts

-        }
-        return v;
+        this._$changeProperty(name, undefined, options, true);
+        return v ?? (options.defaultValue as V);


What do you think about a dev mode warning that they have both a default value and an initializer?

I think it's not really possible to detect for all cases, and having a warning that sometimes shows just seems confusing and hard to maintain.

For example, decorator v. not, accessor v. field, instance shadowing, etc.

I hope in the future we can move to standard decorators only and this will be the place we warn. I guess not that many people would use it now. We can try to add a lint warning somewhere.

packages/reactive-element/src/reactive-element.ts

justinfagnani · 2025-02-03T20:28:02Z

packages/reactive-element/src/reactive-element.ts

+            continue;
+          }
+          const value = this[p as keyof this];
+          // Ensure any initially set value is seen as a change.


Do we want to do this for default values? It kind of makes sense to me to consider the initial default value not changed.

Or maybe that's what this does? Might be clearer if the comments are inside the conditions? If so, maybe mention that in the jsdocs for the defaultValue option too.

Made the comments clearer. Properties with defaultValue set are currently included in initial changes.

We could choose to change this but the number of different ways properties can be configured makes this really hard to manage. (In fact there are some issues with this currently: accessor defined properties are only handled specially with the decorator and not in JS.)

Co-authored-by: Justin Fagnani <justin@fagnani.com>

justinfagnani · 2025-02-04T15:49:23Z

Awesome @sorvell!!

We need to update lit.dev for this feature. Can you do that?

sorvell · 2025-02-04T19:12:41Z

Docs PR: lit/lit.dev#1396

Lookwe69 · 2025-02-04T20:47:06Z

defaultValue shouldn't be a function ? It can be eager when the default value is an object. With a function, we can delay the creation only when needed.

sorvell · 2025-02-14T00:27:49Z

19963dd adds setting the default value when the attribute is removed.

It also changes the strategy used to ensure the default value is the value of the property at construction time. We need to modify the getter to do this since we can't set the value before the user's constructor runs (at least for accessor without standard decorators).

The default value is returned from the getter when it would otherwise return undefined and the setter for the property has been called (this is recorded via a set). Maintaining this set is a slight tax/complication.

Concerns and Alternatives

Since this approach requires both the setter and the getter to be wrapped, these must be separately decorated with standard decorators (by spec). Typescript doesn't allow this when experimental decorators are in use, so this is a migration concern.
Alternatively the initializer could be used as the default value. This would avoid writing the getter and instead just use the normal initializer for it. This would require capturing the default value on the first set call on the property. The concern here is that the storage would be on the instance rather than property options and therefore consume excess memory. Note, while it could technically be on property options, doing so could have unexpected behavior since data would be implicitly shared between instances.
Note also, non-primitive values as defaults doesn't really make sense with the default in property options since the default value is shared between instances. To address this, with either the current defaultValue approach or the capturing initializer approach, we could throw/warn or just not use a non-primitive value.
For defaultValue, ideally, we'd like to warn if the initializer is used, but it's unclear how to detect this case in general.

Adds skipping initial attribute reflection for default values.

30b4399

sorvell assigned justinfagnani Jan 17, 2025

Steve Orvell added 4 commits January 16, 2025 16:28

Only perform attribute check after first update

4612027

Add tests and refine approach

8fcfeb9

Update size, adds 377 bytes

69e3634

Add changeset

cfc08a3

sorvell marked this pull request as ready for review January 19, 2025 00:34

sorvell requested a review from kevinpschaaf as a code owner January 19, 2025 00:34

sorvell requested review from justinfagnani and removed request for kevinpschaaf January 19, 2025 13:34

sorvell mentioned this pull request Jan 20, 2025

Decorator Roadmap lit/rfcs#20

Merged

justinfagnani reviewed Jan 21, 2025

View reviewed changes

packages/reactive-element/src/reactive-element.ts Outdated Show resolved Hide resolved

packages/reactive-element/src/reactive-element.ts Outdated Show resolved Hide resolved

Address feedback, simplify to just defaultValue

07e3761

sorvell requested a review from justinfagnani January 21, 2025 13:40

Update size -51 Bytes

b1c9262

justinfagnani requested changes Feb 3, 2025

View reviewed changes

Steve Orvell and others added 4 commits February 3, 2025 16:21

Merge branch 'main' into default-value

1846f33

Update packages/reactive-element/src/reactive-element.ts

c794eca

Co-authored-by: Justin Fagnani <justin@fagnani.com>

Update packages/reactive-element/src/reactive-element.ts

a503e19

Co-authored-by: Justin Fagnani <justin@fagnani.com>

address review feedback

cfba76f

justinfagnani approved these changes Feb 4, 2025

View reviewed changes

didn't mean to commit

6d6334d

sorvell mentioned this pull request Feb 4, 2025

Add docs for useDefault lit/lit.dev#1396

Merged

justinfagnani changed the title ~~Adds skipping initial attribute reflection for default values.~~ [reactive-element] Adds skipping initial attribute reflection for default values. Feb 12, 2025

justinfagnani mentioned this pull request Feb 12, 2025

[labs/nextjs] reflect: true on props are throwing error #3858

Open

sorvell marked this pull request as draft February 14, 2025 00:03

sorvell and others added 3 commits February 13, 2025 16:06

Merge branch 'main' into default-value

de38864

Fixes initial value and sets default when attr removed

19963dd

update size

57400e9

Steve Orvell added 2 commits February 13, 2025 17:20

fix typo

d41d130

Merge branch 'main' into default-value

3dfc1ce

sorvell mentioned this pull request Feb 19, 2025

Adds useDefault property option #4934

Merged

sorvell closed this Mar 13, 2025

Conversation

sorvell commented Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

justinfagnani commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

justinfagnani Feb 3, 2025

Choose a reason for hiding this comment

Uh oh!

sorvell Feb 4, 2025

Choose a reason for hiding this comment

Uh oh!

justinfagnani Feb 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

justinfagnani Feb 3, 2025

Choose a reason for hiding this comment

Uh oh!

sorvell Feb 4, 2025

Choose a reason for hiding this comment

Uh oh!

justinfagnani commented Feb 4, 2025

Uh oh!

sorvell commented Feb 4, 2025

Uh oh!

Lookwe69 commented Feb 4, 2025

Uh oh!

sorvell commented Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sorvell commented Jan 17, 2025 •

edited

Loading

changeset-bot bot commented Jan 17, 2025 •

edited

Loading

justinfagnani commented Jan 21, 2025 •

edited

Loading

sorvell commented Feb 14, 2025 •

edited

Loading