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

Decorator Roadmap #20

Merged
merged 12 commits into from
Jan 21, 2025
Merged

Decorator Roadmap #20

merged 12 commits into from
Jan 21, 2025

Conversation

justinfagnani
Copy link
Contributor

No description provided.

rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
@rictic
Copy link

rictic commented May 25, 2023

Looks great! A few comments, the only serious one is about context

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! Just some minor clarifying comments.

rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
Copy link
Member

@augustjk augustjk left a comment

Choose a reason for hiding this comment

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

Minor suggestions. Overall LGTM.

rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
@AndrewJakubowicz
Copy link
Contributor

AndrewJakubowicz commented Oct 27, 2023

Thank you @justinfagnani! Applied your changes.

Also ignore the failing CLA. It is not able to figure out personal GitHub emails.

@augustjk
Copy link
Member

Fixed CLA issue by amending commit and force pushing

rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
rfcs/NNNN-decorator-roadmap.md Outdated Show resolved Hide resolved
@justinfagnani
Copy link
Contributor Author

LGTM on allo changes!

Copy link
Member

@sorvell sorvell left a comment

Choose a reason for hiding this comment

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

I think this should be updated based on the review of lit/lit#4895.

@justinfagnani
Copy link
Contributor Author

@sorvell what needs to be updated? This mentions the plan of record that lit/lit#4895 implements, which is to use property options: https://github.com/lit/rfcs/pull/20/files#diff-a406be4bc64f6899edff250e261c51903172e65fe84a676f1dcac6c63c153dbaR119

@sorvell
Copy link
Member

sorvell commented Jan 21, 2025

what needs to be updated?

Nice, I missed that part.

@justinfagnani justinfagnani merged commit 24a4984 into main Jan 21, 2025
1 check passed
@justinfagnani justinfagnani deleted the decorator-roadmap branch January 21, 2025 16:08
@maxpatiiuk
Copy link

Excited for this change - thanks for the continuous improvements!

The code size of components needs to be investigated since the TypeScript emit for standard decorators appears to be larger than experimental decorators. We might want to point this out and not recommend them as the default until browsers ship natively.

Regarding bundle size:

  • These helpers can be externalized from each file (even from the package) using the following:
    • add noEmitHelpers:false and importHelpers:true to your tsconfig.jaon
    • add tslib dependency (not dev dependency) to your app/library package.json
    • This way, all runtime helpers are externalized into the tslib package, ensuring they are included only once in the final build, rather than once per package!

Add a non-core utility library that can dynamically create reactive properties

Very interested in this - could you provide more details on how that may look like and what the tradeoff is?

For reference, this is what we are doing at build time at present:

  • Replace all @property() and @state() decorators with static properties - this way we don't need to ship decorators polyfill
    • This reduced the minified CDN build size in a package with 50 components from 398kb to 364kb (34kb!)
  • To further improve bundle size, rather than declaring options as an object, the build plugin convers them to a very compact representation:
     static {
       this.properties = { autoDestroyDisabled: 5, capabilities: 0, displayType: 1, graphic: 0, hideAddButton: 5, hideAddSubmitButton: 5, hideCancelAddButton: 5, hideCancelUpdateButton: 5, hideDeleteButton: 5, hideErrorMessage: 5, hideProgressBar: 5, hideUpdateButton: 5, icon: 1, label: 1, position: 1, referenceElement: 1, state: 3, submitting: 4 };
     }
  • We overwrite LitElement.createProperty to decode those compact numbers back to options objects. This is done using a typescript enum like this:
     export enum PropertyFlags {
       // Ordered based on frequency of usage to keep flags as short as possible
       ATTRIBUTE = 1 << 0,
       REFLECT = 1 << 1,
       BOOLEAN = 1 << 2,
       NUMBER = 1 << 3,
       STATE = 1 << 4,
       // Not part of vanilla Lit - added by us
       READ_ONLY = 1 << 5,
       // Inverted so that the more common case is "false"
       NO_ACCESSOR = 1 << 6,
     }

Obviously, once static properties goes away, we will look for another solution for declaring properties in a very bundle size efficient way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants