-
Notifications
You must be signed in to change notification settings - Fork 60
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
Add native support for event listeners and attributes #389
Conversation
packages/core/source/types.ts
Outdated
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 contains the source of all the other changes: adding the ability to specify whether an UPDATE_PROPERTY
mutation is setting an instance property, attribute, or event listener, and communicating each of those details separately when serializing remote elements for the host.
@@ -44,6 +44,10 @@ export class Element extends ParentNode { | |||
return attributes; | |||
} | |||
|
|||
getAttributeNames() { |
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 noticed the need for this polyfill when implementing more attribute-related APIs. I think a modern version of React also depended on its presence.
definition.type === Function && definition.name.startsWith('on') | ||
? definition.alias?.[0] | ||
: undefined; | ||
updatedProps[aliasTo ?? prop] = propValue; |
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 now selects the right event based on eventProps
, and creates an on${event}
prop that Preact will set as an event listener.
...attributes.value, | ||
...Object.keys(resolvedEventListeners).reduce<Record<string, any>>( | ||
(listenerProps, event) => { | ||
listenerProps[`on${event[0]!.toUpperCase()}${event.slice(1)}`] = |
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.
Auto-converting event listeners to onX
casing, regardless of the event name. This seems more conventional for React/ Preact, even though in Preact the event listener is added with onx
casing of the prop name.
expect(listener).toHaveBeenCalledWith( | ||
expect.objectContaining({type: 'press', detail}), | ||
); | ||
}); |
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 a minor breaking change but I don't think anyone was actually using it
Haven't looked at the implementation yet, just posting a few examples & notes before I dig in: add-listeners-and-attributes...ja/bubbles-when-shouldnt - in this example, a non-bubbling event is dispatched in the host, but it appears to bubble in the remote. Ideally there shouldn't be a difference of behaviour here. Btw, I find this "minimal-worker" setup useful for testing. The basic iframe demos don't cover the polyfill stuff, which is important to test, whereas the kitchen sink demo is a bit much for low-level testing. Happy to keep this in my branch, but maybe it's useful to others? add-listeners-and-attributes...ja/event-missing - in this example, a bubbling event is dispatched in the host. In the remote, a listener for it is added to a parent element. The listener isn't called. Sorry if this is making wrong assumptions about the implementation, but I wanted to jot this down while it's fresh in my head. Here's a few options I see: Dispatch events eagerlyIf a remote element declares it supports an event, then any instance of that event on the host should be sent to the remote, regardless of whether there's a listener attached. The event on the remote should copy the host in terms of bubbling. This has the following downsides: Events are sent to the worker even if nothing is listening for them. This is mostly an optimisation issue, and I don't know how much it matters. If the host manually changes the event path (ie Synchronise listenersWhen a listener is added to a remote element, it should also be added to the host, even if that element doesn't declare support for that particular event. That means, when I add the listener on This hits the optimised path a bit more, and also 'caters' for For example, if there are listeners on both I guess you could use a My advice would be to go with "Dispatch events eagerly" and look to optimise if it's a performance issue in practice. |
(I still haven't looked at the implementation, just noodling) add-listeners-and-attributes...ja/attribute-change-on-host-missed - here the attribute is changed on the host, but that isn't communicated to the remote. I don't care much about this one. I think it's weird when HTML elements change their own attributes. Although, it does happen. Eg Depends if we care about being compatible with that. |
Again, this isn't something I care a ton about, but thought I'd mention it anyway: class MyElement extends RemoteElement {
static get remoteAttributes() {
return ['label'];
}
} This pattern is similar to custom elements, but I've always found it a bit weird that this ends up part of the public API of the element. An alternative pattern would be: class MyElement extends RemoteElement {
constructor() {
super({
remoteAttributes: ['label'],
});
}
} Although this makes it per instance, rather than per 'class'. |
I think this might be supported, but I couldn't 100% figure it out from the source… is there an ability to create a custom event object in the remote? For example, I'm thinking of the |
2c04268
to
e0c7bd0
Compare
Thanks so much for poking at this @jakearchibald! I pushed a couple of changes I will talk through based on this feedback.
Yes, this was super helpful for debugging changes I made related to this, and agreed that it is much closer to the environment Shopify's version of this will run in.
This was actually a bug in the DOM polyfill, where
I am still kind of worried about the potential waste of sending unnecessary information between remote and host environments. It has never strictly been an issue, but I worry that a slow accretion of sending too much information will have an overall negative effect on extensions as we scale their use. I also think that generally, bubbling event management is not a popular approach to web development, outside of activities like UI instrumentation. I am curious to get your feedback on a middle ground I implemented: a way for developers to specify that a remote event should bubble. This option will both configure "remote events" to be dispatched with import {RemoteElement} from '@remote-dom/core/elements';
// No need to register the `change` event on possible parents
class ParentElement extends RemoteElement {}
customElements.define('parent-element', ParentElement);
// Define the `change` event as bubbling
class MyElement extends RemoteElement {
static get remoteEvents() {
return {
change: {
bubbles: true,
},
};
}
}
customElements.define('my-element', MyElement);
const parent = document.createElement('parent-element');
const element = document.createElement('my-element');
parent.append(element);
parent.addEventListener('change', (event) => {
console.log('Nested element changed!', event.target, event.bubbles);
}); This does stretch the meaning of
Yeah, this one is not intended to be supported. Remote DOM is only offering a one-way remote-to-host communication of the tree. Two-way synchronization could be built on top, but I think I'd encourage folks to use additional RPC channels between the remote and the host (e.g., the API object given to Shopify UI Extensions) for any information they would coordinate that way.
Both the
Yes, for sure. You can use the import {RemoteElement} from '@remote-dom/core/elements';
class ReadyEvent extends CustomEvent {
constructor(port) {
super('ready');
this.port = port;
}
}
class Chat extends RemoteElement {
static get remoteEvents() {
return {
ready: {
dispatchEvent(port) {
// Before calling event listeners, update some properties on the element,
// so they can be read in event listeners.
Object.assign(this, {port});
return new ReadyEvent(port);
},
},
};
}
}
customElements.define('ui-chat', Chat);
const chat = document.createElement('my-chat');
chat.addEventListener('ready', (event) => {
event.port.postMessage('Go!');
}); In this specific case I think we may actually want to switch to using a method on the |
If there a particular high-volume event you're worried about?
My understanding is that React makes extensive use of it under the hood. Eg https://share.descript.com/view/IRpKjmqGwwg. Here's the demo page https://3qwd4m.csb.app/. Given the popularity of React, it feels important that this stuff should work as expected, and we shouldn't cut corners here.
I think that's ok for now.
That's fair. |
b39f518
to
c3fdb67
Compare
@jakearchibald on bubbling: I do agree that we will need to set events like Interestingly, for events that React doesn't have any special handling for, it will not use event delegation, and will just attach the event listener directly to the element. So, for totally custom events we create without a DOM equivalent, I think we are safe to choose whether the event should bubble or not on a case-by-case basis. |
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.
Notes from pairing session with @jakearchibald:
- Does
remoteEvents = ['change']
give aonchange
property? - Use
WeakMap
to store the events and attributes - Need to handle event listener for same event at multiple spots in the tree, without duplicating calls in the remote
<one onclick> <two> <three onclick> <four>
Add `getAttributeNames()` polyfill to support vitest assertions Add event listener support Add proper support for event listeners Clean up some naming Update READMEs Fix Preact tests More documentation More documentation polish Add missing generic argument More docs polish Fix `Event.bubbles` and `Event.composedPath()` implementations Fix missing `connectedCallback()` and `disconnectedCallback)` calls Added `bubbles` configuration option for `RemoteElement` events Fixes to event dispatching (#403) * Make immediatePropogationStopped private-ish * Use workspace polyfill * Stopping immediate propagation also stops regular propagation * Assorted dispatching fixes - Respect stopPropagation throughout both capturing and bubbling - Call listeners on the target element in both the capturing and bubbling phases - Simplify returning defaultPrevented * Add changeset * Better param name * Fix lockfile * Trying again… Revert pnpm fixes This reverts part of commit 0ce1450. Minimal change to pnpm lockfile
Co-authored-by: Jake Archibald <[email protected]>
52a33f2
to
cf953f6
Compare
Previously, Remote DOM only offered “remote properties” as a way to synchronize element state between the host and and remote environments. These remote properties effectively synchronize a subset of a custom element’s instance properties. The
RemoteElement
class offers a declarative way to define the properties that should be synchronized.The same
remoteProperties
configuration can create special handling for attributes and event listeners. By default, a remote property is automatically updated when setting an attribute of the same name:Similarly, a remote property can be automatically updated when adding an event listener based on a conventional
on
property naming prefix:These utilities are handy, but they don’t align with patterns in native DOM elements, particularly when it comes to events. Now, both of these can be represented in a fashion that is more conventional in HTML. The
remoteAttributes
configuration allows you to define a set of element attributes that will be synchronized directly the host environment, instead of being treated as instance properties:Similarly, the
remoteEvents
configuration allows you to define a set of event listeners that will be synchronized directly with the host environment:The
remoteProperties
configuration will continue to be supported for cases where you want to synchronize instance properties. Because instance properties can be any JavaScript type, properties are the highest-fidelity field that can be synchronized between the remote and host environments. However, adding event listeners using theremoteProperties.event
configuration is deprecated and will be removed in the next major version. You should use theremoteEvents
configuration instead. If you were previously defining remote properties which only accepted strings, consider using theremoteAttributes
configuration instead, which stores the value entirely in an HTML attribute instead.This change is being released in a backwards-compatible way, so you can continue to use the existing
remoteProperties
configuration on host and/or remote environments without any code changes.All host utilities have been updated to support the new
attributes
andeventListeners
property that are synchronized with the remote environment. This includes updates to the React and Preact hosts to map events to conventional callback props, and updates to theDOMRemoteReceiver
class, which now applies fields to the host element exactly as they were applied in the remote environment: