-
Notifications
You must be signed in to change notification settings - Fork 1k
[lit-html, reactive-element] Revert change to pause update cycle while disconnected #2034
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
Conversation
🦋 Changeset detectedLatest commit: a54c3dc The changes in this PR will be included in the next version bump. 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 |
📊 Tachometer Benchmark ResultsSummarynop-update
render
update
update-reflect
Resultslit-element-list
render
update
update-reflect
lit-html-kitchen-sink
render
update
nop-update
lit-html-repeat
render
update
lit-html-template-heavy
render
update
reactive-element-list
render
update
update-reflect
|
| parent: Disconnectable, | ||
| attributeIndex: number | undefined | ||
| ) { | ||
| this.isConnected = (part.options as PrivateRenderOptions).isConnected; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider naming the private key in renderOptions more obtusely since users can pass custom values here and we don't want to conflict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's basically fine to conflict if we always overwrite. It's the same as ignore the user-provided value. If we only sometimes overwrote it'd be an issue, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is an implementation detail that we don't want anyone to even be reading from, I went ahead and renamed it to _$isConnected and made it a stableProperty.
Add isConnected handling to SSR Fix SSR test Re-work async directives for better gc behavior Allows the directives (and the DOM they are attached to) to be gc'ed when disconnected before awaited promises/iterables resolve Formatting Rename PrivateRenderOptions.isConnected to _ and make stable Ensure protected methods aren't renamed
5036725 to
422478e
Compare
| /** @internal */ | ||
| _$parent: Disconnectable | undefined; | ||
| /** @internal */ | ||
| __isConnected = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be initialized via options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this distinct from this.options' isConnected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The connection state of the top-level part (the part created/returned by render that the user can call part.setConnection(true/false) on) is the source-of-truth for everything below it. The options._$isConnected flag is not the source of truth, but rather a communication channel between the top-level part and everything below it since they all get references to this one options bag passed down.
Should this be initialized via options?
In theory, the __isConnected flag on any parts below the top-level part returned is never used (and the top-level part should never get an options bag with _$isConnected in it), so it doesn't really matter, but we could keep it in sync just for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a flaw though... It's possible to get a reference to a child part that shares options with the top-level part, and then call part.setConnected(false) and change the connection state. From there, if you did part.setValue(...), directives would still see the top-level connection state, not the local one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per design conversation, re-worked to use get _$isConnected() { return this._$parent._$isConnected } on all Disconnectables up to the RootChildPart, which holds the state for the entire tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And we're TODO'ing setting the initial connected state in render because in LitElement it's almost always going to be first rendered when connected, yeah?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, added TODO's, a skipped failing test for this (https://github.com/lit/lit/pull/2034/files#diff-fc790d086c06e1adeb4c976a60945f0b9b50aa86350a9c9c6b61618c48bac95dR522), and issue #2051
| assert.equal(a.updateCount, 4); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test to verify that updates happen while disconnected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Co-authored-by: Steve Orvell <[email protected]>
packages/lit-html/src/lit-html.ts
Outdated
| * of `AsyncDirective`s created throughout the tree below it. | ||
| */ | ||
| export type {RootChildPart}; | ||
| class RootChildPart extends ChildPart { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could call this just RootPart
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It didn't change the size, so I liked the clarity of RootChildPart.
| // read it once we know the subtree has directives that need | ||
| // to be notified | ||
| let newConnectionState; | ||
| if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this not needed before? Was it just an omission? If so did we add test coverage?
Let's also add a comment here explaining that when a part is moved, its connection state needs to be updated to match the part it's moved into.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not an omission per se; prior to this the user could change any part's connection state, so it was up to the user moving a part to keep it in sync.
With this change, only the root part holds connection state, and everything under it is kept in sync / notified automatically. Added a test in directive-helpers_test.ts.
|
|
||
| constructor(_partInfo: PartInfo) {} | ||
|
|
||
| get _$isConnected() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment here about how this works and why it's here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment
| clearPart(containerPart); | ||
| insertPart(containerPart, undefined, cachedPart); | ||
| setCommittedValue(containerPart, [cachedPart]); | ||
| cachedPart.setConnected(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is removed because insertPart handles it now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right
| if (cachedContainerPart === undefined) { | ||
| const fragment = document.createDocumentFragment(); | ||
| cachedContainerPart = render(nothing, fragment); | ||
| cachedContainerPart.setConnected(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is set earlier here because insertPart synchronizes the connection state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, previously we changed the connection state on the ChildPart that we inserted and moved between the cache and the container parts as we moved them. Now, cachedContainerPart is a RootChildPart, so we set it to "disconnected" once, and each time we move a part in and out, its connection state will be synced to the root part it is in.
| this._$parent = parent; | ||
| } | ||
|
|
||
| get _$isConnected() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment describing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documented once in the interface and added comments for each one pointing to it.
As opposed to other Disconnectables, AsyncDirectives always get notified when the RootChildPart connection changes, so the public `isConnected` available to end users can just be the local value, not a getter.
| newValues[this.__attributeIndex!] = value; | ||
| (this.__part as AttributePart)._$setValue(newValues, this, 0); | ||
| } | ||
| if (isSingleExpression(this.__part as unknown as PartInfo)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs above this still say the value is queued. Need to update that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, done.
|
|
||
| export const memorySuite = canRunMemoryTests ? suite : suite.skip; | ||
|
|
||
| export const forceGC = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still wan to add a comment about why this is being called 10x.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file actually goes with the next PR. Will test there and remove if unneeded.
| } | ||
| } | ||
| /** @internal */ | ||
| setConnected(isConnected: boolean) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should still add some docs that say this should only be called on root parts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| * of `AsyncDirective`s created throughout the tree below it. | ||
| */ | ||
| export interface RootChildPart extends ChildPart { | ||
| setConnected(isConnected: boolean): void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add jsdocs here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
LGTM, though I still prefer the name RootPart. RootChildPart just sounds a bit awkward and contradictory to me. |
|
@justinfagnani Ok I think I changed my mind. I liked |
* Sync disconnection-related docs with changes in lit/#2034 See lit/lit#2034 * Update packages/lit-dev-content/site/docs/templates/custom-directives.md Co-authored-by: Justin Fagnani <[email protected]> * Updates based on feedback * Typos Co-authored-by: Justin Fagnani <[email protected]>
Reverts the change in Lit 2 to pause ReactiveElement's update cycle while the element is disconnected.
This change was originally added to Lit 2 to ensure that once AsyncDirectives are disconnected, they do not receive further
updatesuntil they are reconnected.However, the change in Lit 2 to pause updates while the element was disconnected was found during internal testing to be subtly breaking in potentially dangerous ways, which could lead to memory leaks if the update cycle was relied on to clean up external event handlers/subscriptions.
Instead, the onus will shift to
AsyncDirectiveauthors to check thethis.isConnectedflag duringupdateand avoid doing any potentially leaky work while disconnected, since they may never be reconnected in that state.Note that calling
setValuefrom anasyncDirectivewhile disconnected will now synchronously commit the change (since it seems arbitrary now to not do that; that was only in to maintain the same guarantee we're removing above (that disconnected directives don't receive updates).A follow-on PR (#2044) re-works the disconnection handling for first-party
AsyncDirectives(until,asyncAppend,asyncReplace) to allow them to be GC'ed before their awaited promises resolve.lit.dev documentation updates covering this change are in lit.dev#436.