chore: Apply transformations to make code compatible with 1P#1655
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors various Lit components and tests by updating imports from @a2ui/lit/v0_9 to relative paths and adding the override keyword to overridden methods and properties. It also replaces the use of unsafeStatic with document.createElement in render-a2ui-node.ts. However, a critical issue was identified in render-a2ui-node.ts where recreating the DOM element on every render cycle will cause child components to constantly unmount and remount, resulting in a loss of internal state and severe performance degradation. It is recommended to cache and reuse the created element on the persistent context.componentModel object instead.
ditman
left a comment
There was a problem hiding this comment.
Minor comment about the PR description
| # Local Node environment already installed; invoke standard script targets | ||
| if [ "$CHECK_ONLY" = true ]; then | ||
| yarn format:check:all | ||
| corepack yarn format:check:all |
There was a problem hiding this comment.
Interesting, why not corepack enable at the top of the block?
| @@ -17,7 +17,7 @@ | |||
| import {nothing} from 'lit'; | |||
| import {html, unsafeStatic} from 'lit/static-html.js'; | |||
There was a problem hiding this comment.
It seems this PR does not really address unsafeStatic in any way in the open source version of the repo. Could you update the description to match what it's doing?
There was a problem hiding this comment.
Good catch! I attempted this but was harder than I thought, and then reverted it without updating the description. The description is up to date now.
186ce33 to
16b1fed
Compare
|
Ah, good catch! I've updated |
This PR applies safe, style-only, or best-practice transformations to the codebase, reducing the number of transformations needed when importing to 1P. It covers adding the
overridekeyword, replacing external imports with relative ones, and explicitly typing TextField. The transformation related to stripping theaccessorkeyword was skipped as it is not an inherently backwards-compatible/style change.Next, I'll add enforcing lint enforcing so that we don't regress on this #1667