Fix memory leaks #102
Fix memory leaks #102
Conversation
jculvey
requested review from
petebacondarwin,
IgorMinar and
dario-piotrowicz
| @@ -26,6 +26,8 @@ export function Header({ | |||
| }`} | |||
| placeholder="What needs to be done?" | |||
| autoFocus | |||
| autoComplete="off" | |||
There was a problem hiding this comment.
I believe that this attribute makes no difference. Can you please confirm and remove?
| @@ -26,6 +26,8 @@ export function Header({ | |||
| }`} | |||
| placeholder="What needs to be done?" | |||
| autoFocus | |||
| autoComplete="off" | |||
| spellCheck={false} | |||
There was a problem hiding this comment.
Can you please add a comment with a reference to the chrome big we filed? Thanks
| @@ -60,6 +60,7 @@ export class GenericMessageBus implements MessageBus { | |||
| } | |||
|
|
|||
| dispatch(eventName: string, value: JSONValue) { | |||
| value = JSON.parse(JSON.stringify(value)); | |||
There was a problem hiding this comment.
Can you please add a comment to explain what's going on here?
Also could we use https://developer.mozilla.org/en-US/docs/Web/API/structuredClone instead? (structuredClone feels less hacky)
| @@ -80,7 +80,28 @@ export class PiercingFragmentOutlet extends HTMLElement { | |||
|
|
|||
| disconnectedCallback() { | |||
| if (this.fragmentHost) { | |||
| unmountedFragmentIds.add(this.fragmentHost.fragmentId); | |||
| const fragmentId = this.fragmentHost.fragmentId; | |||
There was a problem hiding this comment.
Can you add a comment before this block explaining what we are doing here? Thanks
| <piercing-fragment-host fragment-id=${fragmentConfig.fragmentId}> | ||
| </piercing-fragment-host> | ||
| ${prePiercingStyles} |
There was a problem hiding this comment.
Ahh... I see. This explains the FOUC
| @@ -463,6 +466,35 @@ function getEscapedReframedClientCode(fragmentId: string) { | |||
| </script>`; | |||
| } | |||
|
|
|||
| function getEmbeddedStyleScript(fragmentId: string) { | |||
There was a problem hiding this comment.
Can you please document the purpose of this fn. thank you
| // monkey patch global addEventListner on window and document so that we can automatically unregister | ||
| // any listeners created from within a registered fragment. | ||
| const addEventListenerTargets = [window, document]; | ||
| // const addEventListenerTargets = [Node.prototype]; |
| if (listenerFns) { | ||
| listenerFns.push({ | ||
| target: addEventListenerTarget, | ||
| // target: this, |
|
|
||
| return originalDocumentAddEventListener.call( | ||
| addEventListenerTarget, | ||
| // this, |
| @@ -368,6 +368,7 @@ export class PiercingGateway<Env> { | |||
| <piercing-fragment-host fragment-id=${fragmentConfig.fragmentId}> | |||
| ${prePiercingStyles} | |||
| --FRAGMENT_CONTENT-- | |||
| <div class="fragment-end"></div> | |||
There was a problem hiding this comment.
interesting.. this works, but it really shouldn't be part of the library. Is there a way to avoid it?
If not can you please add a comment here explaining that the end marker is only used by the seams visualization in this demo?
There was a problem hiding this comment.
I can use body.show-seams piercing-fragment-host:has(*) instead. Dario mentioned a corner case where the fragment host contains non-visible elements (which would still cause the seams to display). I can't repro that with the productivity suite, but it can definitely happen.
I can't really think of an alternative to the above selector that wouldn't involve embedding a sentinel value element that we then query for with a more specific selector. Open to ideas.
Personally, I vote to go with body.show-seams piercing-fragment-host:has(*) for now since it makes the demo code simpler unless we can find a use case within the productivity suite which breaks things.
| @@ -380,6 +381,7 @@ export class PiercingGateway<Env> { | |||
| <iframe id="iframe_${fragmentId}" style="display: none" srcdoc=" | |||
| <body> | |||
| --FRAGMENT_CONTENT-- | |||
| <div class="fragment-end"></div> | |||
Addresses the memory leak issue we found in the reframed demo