[labs/context] Make `@provide` and `@consume` support optional and private properties. by jpzwarte · Pull Request #3733 · lit/lit

jpzwarte · 2023-03-16T14:18:39Z

As far as I can tell, the change just works. All tests still pass.

changeset-bot · 2023-03-16T14:18:42Z

🦋 Changeset detected

Latest commit: 31096c7

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

This PR includes changesets to release 1 package

Name	Type
@lit-labs/context	Patch

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 · 2023-03-17T19:18:30Z

I think the reason I didn't make this change when I did the change for @consume() is that there's the question of whether this provides undefined for a context key that can't be undefined.

const fooContext = createContext<string>('foo');

class Parent extends LitElement {
  @provide({context: fooContext})
  foo?: Foo;
  
  render() { return html`<x-child></x-child>`; }
}

class Child extends LitElement {
  @consume({context: fooContext})
  foo?: Foo;
}

In this case because we have a context of a string and not a string | undefined, if would be an error for this @provide/foo to provide the value.

But that's difficult to differentiate from this case:

const fooContext = createContext<string | undefined>('foo');

class Parent extends LitElement {
  @provide({context: fooContext})
  foo?: Foo;

where it would be ok to provide a value of undefined.

So I think that either:

we can't tell whether to provide a value in this case and we need to keep the current requirement that the field is defined and matches the context type (which you need to include undefined if you want that) and we always provide a value, or
we add a test to this suite that ensures that the consumer gets a number and not undefined, even if we don't set a value like in the other tests. I think we want that test anyway to nail down the intended behavior here.

rictic · 2023-05-24T02:44:35Z

I think we can use a type like this to distinguish between the case where the context allows undefined (in which case an optional field is fine) from the case where it isn't.

type LooseRecord<K, V> = 
    V extends undefined ? 
        Partial<Record<K, V>> : 
        Record<K, V>;

I'll try it out and add some tests

Also add type checking tests

rictic · 2023-05-26T23:33:53Z

This ended up turning into a slightly bigger change, but I added support for providing and consuming optional properties (with correct type checking), and private/protected properties (with no type checking), and with no changes to the authoring experience.

Also added type checking tests using // @ts-expect-error, as well as tests of our behavior when providing and consuming optional properties.

rictic · 2023-05-26T23:45:00Z

Fixes #3883

jpzwarte · 2023-05-27T06:49:09Z

@rictic Thanks for this!

AndrewJakubowicz

Wow! There are some advanced types on display here.

TIL: about how to get around distributive conditional type behavior using square brackets!

Great use of type only tests which are necessary with such a complex change. The two types: FieldMustMatchProvidedType and FieldMustMatchContextType are very similar. Is it possible (or worthwhile) refactoring them into a single type?

AndrewJakubowicz · 2023-05-30T17:20:08Z

.changeset/eighty-bananas-drive.md

+
+Improve type checking of @provide and @consume. We now support optional properties, and public properties should be fully type checked.
+
+This also adds support for using @provide and @consume with protected and private fields, but be aware that do to limitations in TypeScript's experimental decorators, the connection between the context and the property can not be type checked. We'll be able to fix this when standard decorators are released (current ETA TypeScript 5.2 in August 2023).


Nits:

Code blocks around @provide and @consume.

Typo: "but be aware that do due to limitations"

Is there an issue tracking fixing this when standard decorators are released?

This info should also be added to https://lit.dev/docs/data/context/#context-type-checking

Done

Done

Added!

And sent over as lit/lit.dev#1134

AndrewJakubowicz · 2023-05-30T17:27:17Z

packages/labs/context/README.md


 Note that ContextRoot uses [WeakRefs](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakRef) which are not supported in IE11.

+### Protected / Private Properties


Worth mentioning that standard private fields do not work (If I understand correctly)?

AndrewJakubowicz · 2023-05-30T17:30:30Z

packages/labs/context/src/test/context-provider_test.ts

+    assert.strictEqual(consumer.optionalValue, undefined);
+    assert.strictEqual(consumer.consumeOptionalWithDefault, undefined);
+    provider.optionalValue = 500;
+    await provider.updateComplete;


Can this be removed? - noting because in the test above only the consumer is awaited.

Actually both awaits can be removed. Done

AndrewJakubowicz · 2023-05-30T17:32:05Z

packages/labs/context/src/test/type_check_test.ts

+  }
+}
+
+suite('compilation tests', () => {


No action required - is it possible to add a failing compilation test with a standard private field? Maybe linked to a TODO with issue?

AndrewJakubowicz · 2023-05-30T17:33:18Z

packages/labs/context/src/test/type_check_test.ts

+  // this code doesn't need to run, it's tested by tsc
+  if (true as boolean) {
+    return;
+  }


Clever way to skip everything.

AndrewJakubowicz · 2023-05-30T17:53:09Z

packages/labs/context/src/lib/decorators/consume.ts

+  Obj,
+  Key extends PropertyKey,
+  ProvidedType,
+  ReturnValue


If ReturnValue is always void | any, is it something that can be hoisted out? E.g. type ReturnValue = void | any? Otherwise I expected it to possibly be parameterized.

Good point, removed

AndrewJakubowicz · 2023-05-30T18:02:33Z

packages/labs/context/src/lib/decorators/provide.ts

+    : // Next we check whether the object has the property as an optional field
+    Obj extends Partial<Record<Key, infer Providing>>
+    ? // Check assignability again. Note that we have to include undefined
+      // here on the consuming type because it's optional.


Nit: Should this comment reference the providing type?

Good catch!

AndrewJakubowicz · 2023-05-30T18:16:15Z

packages/labs/context/src/lib/decorators/provide.ts

+          context: ContextType;
+          consuming: Providing | undefined;
+        }
+    : // Ok, the field isn't present, so either someone's using consume


Nit: "using ~~consume~~ provide"

Ah, right! Done.

Kept it as a local type alias so that there's a single place to put the comment about why we're using such a strange type.

…ields

Make @provide also support optional properties.

9fd7d12

jpzwarte requested a review from benjamind as a code owner March 16, 2023 14:18

Add changeset.

21d1a83

jpzwarte requested a review from justinfagnani as a code owner March 16, 2023 14:21

jpzwarte changed the title ~~Make @provide also support optional properties.~~ [labs/context] Make @provide also support optional properties. Mar 17, 2023

rictic added 3 commits May 26, 2023 15:56

Improve the type of consume

4afb23c

Also add type checking tests

Support private and protected fields in @provide

9dc2b11

Add test of consume/provide with optionals

37b396a

rictic requested a review from kevinpschaaf as a code owner May 26, 2023 23:30

Update changelog and readme

26c4fa1

rictic changed the title ~~[labs/context] Make @provide also support optional properties.~~ [labs/context] Make @provide and @consume support optional and private properties. May 26, 2023

rictic requested a review from AndrewJakubowicz May 30, 2023 16:56

AndrewJakubowicz approved these changes May 30, 2023

View reviewed changes

rictic mentioned this pull request May 31, 2023

[labs/context] When we move to standard decorators, we should support type checking that a private property decorated with @provide or @consume matches the context type #3926

Open

1 task

rictic added 6 commits May 30, 2023 19:15

Update documentation

d114115

Remove ReturnValue type parameter

16525f8

Kept it as a local type alias so that there's a single place to put the comment about why we're using such a strange type.

consume -> provide

7133afa

Remove unnecessary awaits

e53bec8

Add compilation tests confirming decorators unsupported on #private f…

b22bca8

…ields

Add one more comment.

31096c7

rictic enabled auto-merge (squash) May 31, 2023 02:26

rictic merged commit a7e17ad into lit:main May 31, 2023

This was referenced May 31, 2023

[labs/context] Context decorators error on private or protected fields #3883

Closed

Allow the provide decorator to work on private fields #3889

Closed

This was referenced May 31, 2023

Version Packages #3930

Closed

Version Packages #3932

Merged


		Improve type checking of @provide and @consume. We now support optional properties, and public properties should be fully type checked.

		This also adds support for using @provide and @consume with protected and private fields, but be aware that do to limitations in TypeScript's experimental decorators, the connection between the context and the property can not be type checked. We'll be able to fix this when standard decorators are released (current ETA TypeScript 5.2 in August 2023).


		Note that ContextRoot uses [WeakRefs](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakRef) which are not supported in IE11.

		### Protected / Private Properties

Conversation

jpzwarte commented Mar 16, 2023

Uh oh!

changeset-bot bot commented Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

justinfagnani commented Mar 17, 2023

Uh oh!

rictic commented May 24, 2023

Uh oh!

rictic commented May 26, 2023

Uh oh!

rictic commented May 26, 2023

Uh oh!

jpzwarte commented May 27, 2023

Uh oh!

AndrewJakubowicz 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

changeset-bot bot commented Mar 16, 2023 •

edited

Loading