Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve types for (un)compiled template results #4309

Merged
merged 3 commits into from Nov 3, 2023

Conversation

rictic
Copy link
Collaborator

@rictic rictic commented Oct 19, 2023

Adds two new types: UncompiledTemplateResult and MaybeCompiledTemplateResult. Currently UncompiledTemplateResult is the same as TemplateResult, and MaybeCompiledTemplateResult is the union of the compiled and uncompiled types.

We'd like for code to be able to handle the html and svg functions returning a MaybeCompiledTemplateResult. Changing that type will be a major breaking change though, and we don't want to do a major version so quickly, but this lays the ground work, and provides future-compatible types that people can use when they want to be explicit about which type they mean, because while TemplateResult will change, MaybeCompiled, Uncompiled, and Compiled will all have the same meanings in the next major version.

@changeset-bot
Copy link

changeset-bot bot commented Oct 19, 2023

🦋 Changeset detected

Latest commit: d4291a2

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

This PR includes changesets to release 1 package
Name Type
lit-html 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

@github-actions
Copy link
Contributor

github-actions bot commented Oct 19, 2023

📊 Tachometer Benchmark Results

Summary

nop-update

  • this-change, tip-of-tree, previous-release: unsure 🔍 -5% - +5% (-1.06ms - +1.08ms)
    this-change vs tip-of-tree

render

  • this-change: 76.08ms - 80.69ms
  • this-change, tip-of-tree, previous-release: unsure 🔍 -7% - +6% (-2.19ms - +2.11ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +3% (-1.74ms - +1.68ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +4% (-1.44ms - +1.95ms)
    this-change vs tip-of-tree

update

  • this-change: 855.23ms - 875.61ms
  • this-change, tip-of-tree, previous-release: unsure 🔍 -4% - +4% (-2.80ms - +3.21ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +2% (-2.68ms - +2.41ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +2% (-9.07ms - +15.78ms)
    this-change vs tip-of-tree

update-reflect

  • this-change: 838.03ms - 854.34ms
  • this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +2% (-9.04ms - +19.48ms)
    this-change vs tip-of-tree

Results

this-change

render

VersionAvg timevs
76.08ms - 80.69ms-

update

VersionAvg timevs
855.23ms - 875.61ms-

update-reflect

VersionAvg timevs
838.03ms - 854.34ms-
this-change, tip-of-tree, previous-release

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
31.01ms - 34.41ms-unsure 🔍
-7% - +6%
-2.19ms - +2.11ms
unsure 🔍
-7% - +7%
-2.24ms - +2.32ms
tip-of-tree
tip-of-tree
31.44ms - 34.07msunsure 🔍
-6% - +7%
-2.11ms - +2.19ms
-unsure 🔍
-6% - +6%
-1.93ms - +2.09ms
previous-release
previous-release
31.15ms - 34.19msunsure 🔍
-7% - +7%
-2.32ms - +2.24ms
unsure 🔍
-6% - +6%
-2.09ms - +1.93ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
69.55ms - 73.73ms-unsure 🔍
-4% - +4%
-2.80ms - +3.21ms
unsure 🔍
-4% - +4%
-3.18ms - +2.83ms
tip-of-tree
tip-of-tree
69.28ms - 73.59msunsure 🔍
-4% - +4%
-3.21ms - +2.80ms
-unsure 🔍
-5% - +4%
-3.43ms - +2.67ms
previous-release
previous-release
69.66ms - 73.98msunsure 🔍
-4% - +4%
-2.83ms - +3.18ms
unsure 🔍
-4% - +5%
-2.67ms - +3.43ms
-

nop-update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
19.24ms - 20.79ms-unsure 🔍
-5% - +5%
-1.06ms - +1.08ms
unsure 🔍
-5% - +6%
-1.00ms - +1.14ms
tip-of-tree
tip-of-tree
19.26ms - 20.75msunsure 🔍
-5% - +5%
-1.08ms - +1.06ms
-unsure 🔍
-5% - +6%
-0.99ms - +1.10ms
previous-release
previous-release
19.21ms - 20.69msunsure 🔍
-6% - +5%
-1.14ms - +1.00ms
unsure 🔍
-6% - +5%
-1.10ms - +0.99ms
-
this-change, tip-of-tree, previous-release

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
57.86ms - 60.10ms-unsure 🔍
-3% - +3%
-1.74ms - +1.68ms
unsure 🔍
-0% - +5%
-0.10ms - +2.66ms
tip-of-tree
tip-of-tree
57.71ms - 60.30msunsure 🔍
-3% - +3%
-1.68ms - +1.74ms
-unsure 🔍
-0% - +5%
-0.22ms - +2.84ms
previous-release
previous-release
56.89ms - 58.50msunsure 🔍
-4% - +0%
-2.66ms - +0.10ms
unsure 🔍
-5% - +0%
-2.84ms - +0.22ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
125.73ms - 129.05ms-unsure 🔍
-2% - +2%
-2.68ms - +2.41ms
unsure 🔍
-3% - +1%
-4.07ms - +1.85ms
tip-of-tree
tip-of-tree
125.59ms - 129.45msunsure 🔍
-2% - +2%
-2.41ms - +2.68ms
-unsure 🔍
-3% - +2%
-4.10ms - +2.14ms
previous-release
previous-release
126.05ms - 130.96msunsure 🔍
-1% - +3%
-1.85ms - +4.07ms
unsure 🔍
-2% - +3%
-2.14ms - +4.10ms
-
this-change, tip-of-tree, previous-release

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
52.44ms - 54.49ms-unsure 🔍
-3% - +4%
-1.44ms - +1.95ms
unsure 🔍
-3% - +2%
-1.66ms - +1.22ms
tip-of-tree
tip-of-tree
51.86ms - 54.56msunsure 🔍
-4% - +3%
-1.95ms - +1.44ms
-unsure 🔍
-4% - +2%
-2.16ms - +1.21ms
previous-release
previous-release
52.68ms - 54.69msunsure 🔍
-2% - +3%
-1.22ms - +1.66ms
unsure 🔍
-2% - +4%
-1.21ms - +2.16ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
881.66ms - 897.89ms-unsure 🔍
-1% - +2%
-9.07ms - +15.78ms
unsure 🔍
-2% - +1%
-14.75ms - +8.05ms
tip-of-tree
tip-of-tree
877.01ms - 895.82msunsure 🔍
-2% - +1%
-15.78ms - +9.07ms
-unsure 🔍
-2% - +1%
-19.05ms - +5.65ms
previous-release
previous-release
885.11ms - 901.13msunsure 🔍
-1% - +2%
-8.05ms - +14.75ms
unsure 🔍
-1% - +2%
-5.65ms - +19.05ms
-

update-reflect

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
876.82ms - 897.26ms-unsure 🔍
-1% - +2%
-9.04ms - +19.48ms
unsure 🔍
-1% - +2%
-11.10ms - +16.99ms
tip-of-tree
tip-of-tree
871.88ms - 891.76msunsure 🔍
-2% - +1%
-19.48ms - +9.04ms
-unsure 🔍
-2% - +1%
-16.11ms - +11.56ms
previous-release
previous-release
874.47ms - 893.73msunsure 🔍
-2% - +1%
-16.99ms - +11.10ms
unsure 🔍
-1% - +2%
-11.56ms - +16.11ms
-

tachometer-reporter-action v2 for Benchmarks

@github-actions
Copy link
Contributor

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

@rictic rictic force-pushed the explicit-template-result-compiledness branch from c94a54f to 51abba4 Compare October 24, 2023 23:18
* In Lit 4, this type will be an alias of
* MaybeCompiledTemplateResult, so that code will get type errors if it assumes
* that Lit templates are not compiled. When deliberately working with only
* one, use either CompiledTemplateResult or UncompiledTemplateResult
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Could make CompiledTemplateResult and UncompiledTemplateResult use @linkcode so they link to the respective docs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice!

// This property needs to remain unminified.
['_$litType$']: T;
strings: TemplateStringsArray;
values: unknown[];
};

export type MaybeCompiledTemplateResult<T extends ResultType = ResultType> =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This could use a JSDoc to clarify why this is used. Probably want to call out that this should ideally be the type of html or svg when you're trying to target both uncompiled and compiled environments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Done

Adds two new types: `UncompiledTemplateResult` and `MaybeCompiledTemplateResult`. Currently `UncompiledTemplateResult` is the same as `TemplateResult`, and `MaybeCompiledTemplateResult` is the union of the compiled and uncompiled types.

We'd like for code to be able to handle the `html` and `svg` functions returning a MaybeCompiledTemplateResult. Changing that type will be a major breaking change though, and we don't want to do a major version so quickly, but this lays the ground work, and provides future-compatible types that people can use when they want to be explicit about which type they mean, because while TemplateResult will change, MaybeCompiled, Uncompiled, and Compiled will all have the same meanings in the next major version.
@rictic rictic force-pushed the explicit-template-result-compiledness branch from 51abba4 to d4291a2 Compare October 31, 2023 20:38
Copy link
Contributor

@AndrewJakubowicz AndrewJakubowicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@rictic rictic merged commit 949a546 into main Nov 3, 2023
6 of 7 checks passed
@rictic rictic deleted the explicit-template-result-compiledness branch November 3, 2023 19:38
This was referenced Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants