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

Fixing connected, insert, & remove stuff #446

Merged
merged 7 commits into from
Sep 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fair-lobsters-pull.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@remote-dom/polyfill': patch
---

Ensure that the insert and remove hooks are only called for element parents.
5 changes: 5 additions & 0 deletions .changeset/healthy-rocks-rule.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@remote-dom/polyfill': patch
---

Make connectedCallback and disconnectedCallback call on connect/disconnect recursively
5 changes: 5 additions & 0 deletions .changeset/short-cougars-sparkle.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@remote-dom/polyfill': minor
---

Implement node.isConnected
2 changes: 2 additions & 0 deletions documentation/migrations/remote-ui-to-remote-dom.md
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ icon.setAttribute('type', 'archive');
icon.setAttribute('slot', 'icon');
button.append(icon);
root.append(button);
document.body.append(root);
```

## Update React integration
Expand Down Expand Up @@ -211,6 +212,7 @@ createRoot(remoteRoot).render(<App />);
import {createRoot} from 'react-dom/client';

const root = document.createElement('remote-root');
document.body.append(root);
createRoot(root).render(<App />);
```

Expand Down
1 change: 1 addition & 0 deletions examples/kitchen-sink/app/remote/worker/sandbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ new ThreadWebWorker<never, SandboxAPI>(self as any as Worker, {
// synchronizing its children over a `RemoteConnection`.
const root = document.createElement('remote-root');
root.connect(connection);
document.body.append(root);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lemonmade this is required for custom elements within the root to fire connectedCallback.


await render(root, api);
},
Expand Down
1 change: 1 addition & 0 deletions packages/core/source/tests/elements.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1279,6 +1279,7 @@ function createAndConnectRemoteRootElement() {
const root = createRemoteRootElement();
const receiver = new TestRemoteReceiver();
root.connect(receiver.connection);
document.body.append(root);
return {root, receiver};
}

Expand Down
1 change: 0 additions & 1 deletion packages/polyfill/source/Attr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ export class Attr extends Node {
[NEXT]: Attr | null = null;
[VALUE]: string;
[OWNER_ELEMENT]: Element | null = null;
isConnected = false;

constructor(name: string, value: string, namespace?: NamespaceURI | null) {
super();
Expand Down
3 changes: 3 additions & 0 deletions packages/polyfill/source/Document.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
NodeType,
OWNER_DOCUMENT,
HOOKS,
IS_CONNECTED,
} from './constants.ts';
import type {Window} from './Window.ts';
import type {Node} from './Node.ts';
Expand All @@ -28,6 +29,7 @@ export class Document extends ParentNode {
head: HTMLHeadElement;
documentElement: HTMLHtmlElement;
defaultView: Window;
[IS_CONNECTED] = true;

constructor(defaultView: Window) {
super();
Expand All @@ -37,6 +39,7 @@ export class Document extends ParentNode {
this.body = setupElement(new HTMLBodyElement(), this, 'body');
this.head = setupElement(new HTMLHeadElement(), this, 'head');

this.appendChild(this.documentElement);
this.documentElement.appendChild(this.head);
this.documentElement.appendChild(this.body);
}
Expand Down
17 changes: 10 additions & 7 deletions packages/polyfill/source/Node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
NamespaceURI,
NodeType,
HOOKS,
IS_CONNECTED,
} from './constants.ts';
import type {Document} from './Document.ts';
import type {ParentNode} from './ParentNode.ts';
Expand All @@ -17,6 +18,7 @@ import {
isParentNode,
isTextNode,
cloneNode,
descendants,
} from './shared.ts';

export class Node extends EventTarget {
Expand All @@ -28,6 +30,7 @@ export class Node extends EventTarget {
[CHILD]: Node | null = null;
[PREV]: Node | null = null;
[NEXT]: Node | null = null;
[IS_CONNECTED] = false;

protected get [HOOKS]() {
return this[OWNER_DOCUMENT].defaultView[HOOKS];
Expand All @@ -45,6 +48,10 @@ export class Node extends EventTarget {
return this[OWNER_DOCUMENT];
}

get isConnected() {
return this[IS_CONNECTED];
}

isDefaultNamespace(namespace: string) {
return namespace === NamespaceURI.XHTML;
}
Expand Down Expand Up @@ -113,17 +120,13 @@ export class Node extends EventTarget {
get textContent(): string | null {
if (isCharacterData(this)) return this.data;
let text = '';
function walk(node: Node) {

for (const node of descendants(this)) {
if (isTextNode(node)) {
text += node.data;
}
const child = node[CHILD];
if (child) walk(child);
const sibling = node[NEXT];
if (sibling) walk(sibling);
}
const child = this[CHILD];
if (child) walk(child);

return text;
}

Expand Down
26 changes: 22 additions & 4 deletions packages/polyfill/source/ParentNode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@ import {
OWNER_DOCUMENT,
NodeType,
HOOKS,
IS_CONNECTED,
} from './constants.ts';
import type {Node} from './Node.ts';
import {ChildNode, toNode} from './ChildNode.ts';
import {NodeList} from './NodeList.ts';
import {querySelectorAll, querySelector} from './selectors.ts';
import {selfAndDescendants} from './shared.ts';

export class ParentNode extends ChildNode {
readonly childNodes = new NodeList();
Expand Down Expand Up @@ -66,8 +68,16 @@ export class ParentNode extends ChildNode {
children.splice(children.indexOf(child), 1);
}

(child as any).disconnectedCallback?.();
this[HOOKS].removeChild?.(this as any, child as any, childNodesIndex);
if (this[IS_CONNECTED]) {
for (const node of selfAndDescendants(child)) {
node[IS_CONNECTED] = false;
(node as any).disconnectedCallback?.();
}
}

if (this.nodeType === NodeType.ELEMENT_NODE) {
this[HOOKS].removeChild?.(this as any, child as any, childNodesIndex);
}
}

replaceChild(newChild: Node, oldChild: Node) {
Expand Down Expand Up @@ -154,7 +164,15 @@ export class ParentNode extends ChildNode {
if (isElement) this.children.push(child);
}

(child as any).connectedCallback?.();
this[HOOKS].insertChild?.(this as any, child as any, insertIndex);
if (this[IS_CONNECTED]) {
for (const node of selfAndDescendants(child)) {
node[IS_CONNECTED] = true;
(node as any).connectedCallback?.();
}
}

if (this.nodeType === NodeType.ELEMENT_NODE) {
this[HOOKS].insertChild?.(this as any, child as any, insertIndex);
}
}
}
1 change: 1 addition & 0 deletions packages/polyfill/source/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export const PATH = Symbol('path');
export const STOP_IMMEDIATE_PROPAGATION = Symbol('stop_immediate_propagation');
export const CONTENT = Symbol('content');
export const HOOKS = Symbol('hooks');
export const IS_CONNECTED = Symbol('is_connected');

// @TODO remove explicit values
export const enum NodeType {
Expand Down
29 changes: 28 additions & 1 deletion packages/polyfill/source/shared.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
import {DATA, OWNER_DOCUMENT, ATTRIBUTES, NodeType} from './constants.ts';
import {
DATA,
OWNER_DOCUMENT,
ATTRIBUTES,
NodeType,
CHILD,
NEXT,
} from './constants.ts';
import type {Document} from './Document.ts';
import type {DocumentFragment} from './DocumentFragment.ts';
import type {Node} from './Node.ts';
Expand Down Expand Up @@ -78,3 +85,23 @@ export function cloneNode(
return cloned;
}
}

export function descendants(node: Node) {
const nodes: Node[] = [];
Copy link
Member

Choose a reason for hiding this comment

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

Could we take this extraction as a time to switch this to return an iterator instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the same, but iterators are pretty bad in terms of performance, and my gut feeling is that the performance could matter in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://jsperf.app/dedosu - quick performance test for this. Iterators are about half speed in Chrome, but much slower in Safari and Firefox. It might not matter much in reality. The advantage of the iterator is early breaking, but we don't have that need yet.

const walk = (node: Node) => {
nodes.push(node);
const child = node[CHILD];
if (child) walk(child);
const sibling = node[NEXT];
if (sibling) walk(sibling);
};
const child = node[CHILD];
if (child) walk(child);
return nodes;
}

export function selfAndDescendants(node: Node) {
const nodes = descendants(node);
nodes.unshift(node);
return nodes;
}
Loading