add: event support to @lwc/engine-server #2920

divmain · 2022-07-06T06:30:12Z

Details

This adds support for dispatching events during SSR. Everything is synchronous, and only the bubble phase of event propagation is supported.

This implementation seems to be working as expected in the LWR PoC, so I'd like to start working on tests. However, before doing that, I'd like feedback on the implementation in case folks want to see changes.

Before this PR can be merged, the following tasks need to be completed:

add full test coverage
drop the canary-ssr version bump commits
delete ephemeral-notes.md

Does this pull request introduce a breaking change?

✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

⚠️ Yes, it does include an observable change.

GUS work item

pmdartus · 2022-07-06T09:31:01Z

ephemeral-notes.md

+- **What version of Node is required?** Node >= 16
+- **Why redefine types in `engine-server` that are implicit DOM types (see `lib.dom.d.ts`)?** Unfortunately, it is not permitted to import specific types explicity. Typescript assumes that if you want one DOM type, you are in a browser and will want _all_ DOM types to be implicityly available globally. That may lead to confusion in the future when browser-only types are available in Node-only code. The best alternative is to copy the types we need into `engine-server`.
+- **Which DOM event phases will be supported?** At present, there does not appear to be a use case for supporting the `capture` or `target` phases. As such, server-side DOM-like events will only support the `bubble` phase of event propagation.
+- **Is `Event#preventDefault` handled in any way?** No. Firstly, we're not providing an actual `Event` type - that's up to consumers. Secondly, there is no default behavior to prevent, so this isn't a consideration in the SSR context.


we're not providing an actual Event type

Should we? Consumers would not only have to provide Event, but also CustomEvent. Is the goal to rely on node global Event constructor that has been introduced in Node v15?

On the same topic, Event.prototype.target and Event.prototype.composedPath() are 2 of the most used properties on dispatched events. Does it make sense to implement those properties on SSR? If yes, it means that we would have to expose the internal HostElement to userland code. This would certainly require a better encapsulation of the HostElement internal properties.

packages/@lwc/engine-server/src/renderer.ts

pmdartus · 2022-07-06T09:44:56Z

packages/@lwc/engine-server/src/renderer.ts

+                }
+            }
+        }
+        currentNode = currentNode.parent;


The event should only invoke all the ancestor elements if bubbles is set to true.

The behavior of bubbles: false, composed: true is especially counter-intuitive here. I had to look at PM's post a few times before I got it. 😆

[W]hen the dispatched event is composed only, the event propagates from one host element to another [...] without propagating through the intermediary nodes.

I unresolved this convo, because I don't think the logic is correct for the case @nolanlawson mentioned bubbles: false, composed: true. If bubbles is false, stop is true which prevents the event to propagate to the closest shadow root ancestor.

Thank you for this, btw. I'll make sure there's good test coverage here.

nolanlawson · 2022-07-06T17:45:55Z

ephemeral-notes.md

+- **Why redefine types in `engine-server` that are implicit DOM types (see `lib.dom.d.ts`)?** Unfortunately, it is not permitted to import specific types explicity. Typescript assumes that if you want one DOM type, you are in a browser and will want _all_ DOM types to be implicityly available globally. That may lead to confusion in the future when browser-only types are available in Node-only code. The best alternative is to copy the types we need into `engine-server`.
+- **Which DOM event phases will be supported?** At present, there does not appear to be a use case for supporting the `capture` or `target` phases. As such, server-side DOM-like events will only support the `bubble` phase of event propagation.
+- **Is `Event#preventDefault` handled in any way?** No. Firstly, we're not providing an actual `Event` type - that's up to consumers. Secondly, there is no default behavior to prevent, so this isn't a consideration in the SSR context.
+


One question I have reading this is how is this related to prior art, e.g. Lit: lit/lit#2309

I.e. is our approach compatible with theirs? Could we extract it into a standalone package that would be sharable with other custom element frameworks?

packages/@lwc/engine-server/package.json

nolanlawson · 2022-07-06T18:11:54Z

packages/@lwc/engine-server/src/renderer.ts

+    let savedCallback = callback;
+
+    if (useCaptureOrOptions) {
+        if (useCaptureOrOptions === true || useCaptureOrOptions.capture) {


The spec is not clear on this, but browsers also allow truthy objects like 1 and "foo" here as well. This gets weird because the empty object {}, however, is treated as an actual property bag (i.e. capture: false rather than capture: true).

There is some background discussion on this in WICG/EventListenerOptions#21 (comment) and this Chromium issue.

I was a bit unsure what behavior they landed on, so I just wrote a small test. Luckily all 3 browser engines behave the same. Basically useCapture is true in these cases:

true

1

"foo" (non-empty string)

{ capture: true }

It's false in all the other cases I tested (including truthy values like {} and []).

This is super helpful, thank you! I'll add these conditions w/ comments.

nolanlawson · 2022-07-06T18:15:25Z

packages/@lwc/engine-server/src/renderer.ts

-const removeEventListener = noop as (
-    target: HostNode,
+    if (!(type in node.eventListeners)) {
+        node.eventListeners[type] = new Set();


I would suggest we use a Map here for eventListeners instead of an object. Otherwise weird things can happen if a developer does node.addEventListener('constructor', ...) or node.addEventListener('__proto__', ...).

Probably it's not vulnerable to any kind of security issue (e.g. prototype pollution), but why take the risk? 🙂

nolanlawson · 2022-07-06T18:17:17Z

packages/@lwc/engine-server/src/renderer.ts

+    if (node.type !== 'element') {
+        return;
+    }
+    const eventListeners = node.eventListeners[type];


Nitpick: I find it a bit confusing that eventListeners can refer to both a Record<string, Set> and the Set. Maybe we can call this variable listenersSet or something? Or rename node.eventListeners?

caridy · 2022-08-23T14:55:26Z

I'm still very much confused about what this does, and why it does it?

divmain · 2022-11-09T19:23:55Z

Closing this. After discussion on the team, we will be disallowing event support on the server. And we'll add a separate engine-server implementation for wire context.

divmain added 7 commits July 5, 2022 22:18

feat: event support subset for engine-server (SSR)

740ef37

docs: add ephemeral notes

8eab3c3

fix: account for parent nodes that are shadow roots

8f94c49

docs: add note about which Node version is supported

7eab879

add: support for composed:true events

f284e9a

chore: canary-ssr release

2dc9b6a

chore: fix typo

316a2f8

divmain requested review from caridy, nolanlawson, jodarove, pmdartus and lpomerleau July 6, 2022 06:30

divmain marked this pull request as draft July 6, 2022 06:30

docs: comment

3d8b452

pmdartus reviewed Jul 6, 2022

View reviewed changes

nolanlawson reviewed Jul 6, 2022

View reviewed changes

packages/@lwc/engine-server/package.json Show resolved Hide resolved

nolanlawson reviewed Jul 6, 2022

View reviewed changes

divmain added 2 commits July 6, 2022 11:26

fix: events bubbling when bubbles is falsey

aa647f3

fix: event listeners' this context

359b2fc

divmain closed this Nov 9, 2022

ravijayaramappa deleted the dbustad/engine-server-events branch December 13, 2022 23:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add: event support to @lwc/engine-server #2920

add: event support to @lwc/engine-server #2920

divmain commented Jul 6, 2022 •

edited

Loading

pmdartus Jul 6, 2022

pmdartus Jul 6, 2022

nolanlawson Jul 6, 2022

pmdartus Jul 7, 2022

divmain Jul 14, 2022

nolanlawson Jul 6, 2022 •

edited

Loading

nolanlawson Jul 6, 2022

divmain Jul 6, 2022

nolanlawson Jul 6, 2022

nolanlawson Jul 6, 2022

caridy commented Aug 23, 2022

divmain commented Nov 9, 2022

add: event support to @lwc/engine-server #2920

add: event support to @lwc/engine-server #2920

Conversation

divmain commented Jul 6, 2022 • edited Loading

Details

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

GUS work item

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolanlawson Jul 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caridy commented Aug 23, 2022

divmain commented Nov 9, 2022

divmain commented Jul 6, 2022 •

edited

Loading

nolanlawson Jul 6, 2022 •

edited

Loading