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

feat: add lwc:on directive #5228

Open
wants to merge 47 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 41 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
af5ec53
test(integration-karma): add integration tests for lwc:on directive
gaurav-rk9 Feb 18, 2025
43645d6
feat(engine-core): add optional dynamicOn property to VNodeData inter…
gaurav-rk9 Feb 22, 2025
e448481
feat(engine-core): add patching utilities for dynamic event listeners
gaurav-rk9 Feb 22, 2025
d80984f
feat(engine-core): update hydration and rendering to patch dynamic ev…
gaurav-rk9 Feb 22, 2025
aec5dc7
feat(shared): export Detached Object.prototype.propertyIsEnumerable f…
gaurav-rk9 Feb 23, 2025
96df273
fix(engine-core): use propertyIsEnumerable from @lwc/shared
gaurav-rk9 Feb 23, 2025
154a932
feat(template-compiler): add type and helper functions for OnDirectiv…
gaurav-rk9 Feb 27, 2025
69bb3c2
feat: throw error when lwc:on is used on a non-root <template> element
gaurav-rk9 Feb 27, 2025
d6a0a6a
feat: add parsing of lwc:on directive
gaurav-rk9 Feb 27, 2025
80b6a07
feat: throw error when declarative event listeners are used alongsid…
gaurav-rk9 Feb 28, 2025
c665ac0
feat(template-compiler): add code generation for lwc:on directive
gaurav-rk9 Feb 28, 2025
c6721f9
feat(engine-core): add basic implementation of freeze render API
gaurav-rk9 Feb 28, 2025
52516ee
docs(engine-core): add comment in patchDynamicEventListeners regardin…
gaurav-rk9 Feb 28, 2025
e036dd9
Apply suggestions from code review
gaurav-rk9 Mar 2, 2025
3b7d35a
test(integration-karma): fix ignored properties and event type case t…
gaurav-rk9 Mar 2, 2025
35723ab
test(integration-karma): add tests to verify re-rendering behaviour
gaurav-rk9 Mar 2, 2025
8a26014
test(integration-karma): add a seperate test for object passed as pub…
gaurav-rk9 Mar 2, 2025
e4a492d
fix: fix @api issues by removing freeze api and adding copy api
gaurav-rk9 Mar 2, 2025
e2362b8
style: apply suggestion from code review - consistent error names
gaurav-rk9 Mar 3, 2025
46ebc04
test(integration-karma): add missing test component x/publicProp
gaurav-rk9 Mar 3, 2025
785100a
style(engine-core): rename variables related to patching and add comm…
gaurav-rk9 Mar 3, 2025
eac5f81
fix(engine-core): add missing negation operator in patchDynamicEventL…
gaurav-rk9 Mar 3, 2025
e911556
docs(template-compiler): add comment explaining the check used to kno…
gaurav-rk9 Mar 3, 2025
ef2b742
docs(template-compiler): add comment explaining onDirective codegen i…
gaurav-rk9 Mar 3, 2025
884037c
fix(engine-core): change prevInp to prevOut in comparision
gaurav-rk9 Mar 3, 2025
2ee9351
test(integration-karma): expand re-render tests to include the case w…
gaurav-rk9 Mar 3, 2025
56eed25
test(template-compiler): add snapshot tests for template-compiler
gaurav-rk9 Mar 4, 2025
82cd9c8
test(integration-karma): move catchUnhandledRejectionsAndError inside…
gaurav-rk9 Mar 4, 2025
efa9a5a
test(integration-karma): add re-render tests for lwc:on used inside f…
gaurav-rk9 Mar 4, 2025
528f63a
test(integration-karma): correct 'this' in rerenderLoop.js
gaurav-rk9 Mar 4, 2025
c4cdbd5
fix: move copy api's work to codeGen and patching
gaurav-rk9 Mar 4, 2025
7603842
refactor: remove dead code
gaurav-rk9 Mar 4, 2025
724f925
fix: Apply suggestions from code review - <> in error message
gaurav-rk9 Mar 5, 2025
cb1645c
feat: add template-compiler flag enableLwcOn
gaurav-rk9 Mar 5, 2025
d2f2ce8
test(integration-karma): modify test to include <> in error message
gaurav-rk9 Mar 5, 2025
2535c10
Merge branch 'master' into lwc_on
gaurav-rk9 Mar 5, 2025
88e9a0f
test(integration-karma): remove console log spy
gaurav-rk9 Mar 5, 2025
abcce1b
perf(engine-core): change delete obj[x] to obj[x] = undefined
gaurav-rk9 Mar 5, 2025
021a922
test(integration-karma): simplify tests by re-using spies
gaurav-rk9 Mar 5, 2025
247e660
chore: increase allowed bundlesize limit
gaurav-rk9 Mar 5, 2025
528b1ca
test(template-compiler): fix error codes in snapshots
gaurav-rk9 Mar 5, 2025
1a5236b
fix: change throw new Error to logError
gaurav-rk9 Mar 5, 2025
2e5bf3d
docs(engine-core): add comments explaining dynamicOn has null proto a…
gaurav-rk9 Mar 5, 2025
5a2358b
test(integration-karma): add tests for objects with computed key
gaurav-rk9 Mar 6, 2025
9596f94
fix(engine-core): modify patch logic to differentiate prop value = un…
gaurav-rk9 Mar 6, 2025
e5a2cda
test(integration-karma): add tests where arg has property with value …
gaurav-rk9 Mar 6, 2025
523ac07
test(integration-karma): simplify value evaluation throws tests
gaurav-rk9 Mar 6, 2025
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
3 changes: 3 additions & 0 deletions packages/@lwc/compiler/src/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,8 @@ export interface TransformOptions {
customRendererConfig?: CustomRendererConfig;
/** @deprecated Ignored by compiler. `lwc:spread` is always enabled. */
enableLwcSpread?: boolean;
/** Flag to enable usage of dynamic event listeners (lwc:on) directive in HTML template */
enableLwcOn?: boolean;
/** Set to true if synthetic shadow DOM support is not needed, which can result in smaller/faster output. */
disableSyntheticShadowSupport?: boolean;
/**
Expand All @@ -148,6 +150,7 @@ type OptionalTransformKeys =
| 'scopedStyles'
| 'customRendererConfig'
| 'enableLwcSpread'
| 'enableLwcOn'
| 'enableLightningWebSecurityTransforms'
| 'enableDynamicComponents'
| 'experimentalDynamicDirective'
Expand Down
2 changes: 2 additions & 0 deletions packages/@lwc/compiler/src/transformers/template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ export default function templateTransform(
customRendererConfig,
enableDynamicComponents,
experimentalDynamicDirective: deprecatedDynamicDirective,
enableLwcOn,
instrumentation,
namespace,
name,
Expand All @@ -61,6 +62,7 @@ export default function templateTransform(
enableStaticContentOptimization,
customRendererConfig,
enableDynamicComponents,
enableLwcOn,
instrumentation,
apiVersion,
disableSyntheticShadowSupport,
Expand Down
2 changes: 2 additions & 0 deletions packages/@lwc/engine-core/src/framework/hydration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import { VNodeType, isVStaticPartElement } from './vnodes';

import { patchProps } from './modules/props';
import { applyEventListeners } from './modules/events';
import { patchDynamicEventListeners } from './modules/dynamic-events';
import { hydrateStaticParts, traverseAndSetElements } from './modules/static-parts';
import { getScopeTokenClass } from './stylesheet';
import { renderComponent } from './component';
Expand Down Expand Up @@ -517,6 +518,7 @@ function handleMismatch(node: Node, vnode: VNode, renderer: RendererAPI): Node |

function patchElementPropsAndAttrsAndRefs(vnode: VBaseElement, renderer: RendererAPI) {
applyEventListeners(vnode, renderer);
patchDynamicEventListeners(null, vnode, renderer, vnode.owner);
patchProps(null, vnode, renderer);
// The `refs` object is blown away in every re-render, so we always need to re-apply them
applyRefs(vnode, vnode.owner);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
/*
* Copyright (c) 2025, salesforce.com, inc.
* All rights reserved.
* SPDX-License-Identifier: MIT
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/
import { isUndefined } from '@lwc/shared';
import { EmptyObject } from '../utils';
import { invokeEventListener } from '../invoker';
import type { VM } from '../vm';
import type { VBaseElement } from '../vnodes';
import type { RendererAPI } from '../renderer';

export function patchDynamicEventListeners(
oldVnode: VBaseElement | null,
vnode: VBaseElement,
renderer: RendererAPI,
owner: VM
) {
const {
elm,
data: { dynamicOn, dynamicOnRaw },
sel,
} = vnode;

const oldDynamicOn = oldVnode?.data?.dynamicOn ?? EmptyObject;
const newDynamicOn = dynamicOn ?? EmptyObject;

// Compare raw values to check if same object is passed to lwc:on
const isObjectSame = oldVnode?.data?.dynamicOnRaw === dynamicOnRaw;

const { addEventListener, removeEventListener } = renderer;
const attachedEventListeners = getAttachedEventListeners(owner, elm!);

// Properties that are present in 'oldDynamicOn' but not in 'newDynamicOn'
for (const eventType in oldDynamicOn) {
if (!(eventType in newDynamicOn)) {
// Throw if same object is passed
if (isObjectSame) {
throw new Error(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like, historically, we've made things like this just console warnings that only throw in dev mode. I also feel like, currently, we've decided that's an annoying pattern that nobody pays attention to, so maybe we should just go with this. @divmain, thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, we have typically used logError for this. But in this case, I think I prefer throwing an error - as you say, nobody pays attention to the warnings. And if we don't throw, somebody will eventually do the wrong thing and expect us to make it work.

Copy link
Author

@gaurav-rk9 gaurav-rk9 Mar 5, 2025

Choose a reason for hiding this comment

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

I didn't notice earlier that logError is used elsewhere, In that case I would want to change this to logError since we would want that (reactivity with same object, modified) to be available if need has been demonstrated.
Also, that case (same object passed to lwc:on) works just as fine, if we don't throw, so there is no cost of 'making it work'.
@divmain , is it fine if I make the change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes feel free.

Copy link
Author

Choose a reason for hiding this comment

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

done

`Detected mutation of property '${eventType}' in the object passed to lwc:on for <${sel}>. Reusing the same object with modified properties is prohibited. Please pass a new object instead.`
);
}

// Remove listeners that were attached previously but don't have a corresponding property in `newDynamicOn`
const attachedEventListener = attachedEventListeners[eventType];
removeEventListener(elm, eventType, attachedEventListener!);
attachedEventListeners[eventType] = undefined;
}
}

// Ensure that the event listeners that are attached match what is present in `newDynamicOn`
for (const eventType in newDynamicOn) {
const oldCallback = oldDynamicOn[eventType];
const newCallback = newDynamicOn[eventType];

// Properties that are present in 'newDynamicOn' but whose value are different from that in `oldDynamicOn`
if (oldCallback !== newCallback) {
// Throw if same object is passed
if (isObjectSame) {
throw new Error(
`Detected mutation of property '${eventType}' in the object passed to lwc:on for <${sel}>. Reusing the same object with modified properties is prohibited. Please pass a new object instead.`
);
}

// Remove listener that was attached previously
if (oldCallback) {
const attachedEventListener = attachedEventListeners[eventType];
removeEventListener(elm, eventType, attachedEventListener!);
}

// Bind callback to owner component and add it as listener to element
const newBoundEventListener = bindEventListener(owner, newCallback);
addEventListener(elm, eventType, newBoundEventListener);

// Store the newly added eventListener
attachedEventListeners[eventType] = newBoundEventListener;
}
}
}

function getAttachedEventListeners(
vm: VM,
elm: Element
): Record<string, EventListener | undefined> {
let attachedEventListeners = vm.attachedEventListeners.get(elm);
if (isUndefined(attachedEventListeners)) {
attachedEventListeners = {};
vm.attachedEventListeners.set(elm, attachedEventListeners);
}
return attachedEventListeners;
}

function bindEventListener(vm: VM, fn: EventListener): EventListener {
return function (event: Event) {
invokeEventListener(vm, fn, vm.component, event);
};
}
2 changes: 2 additions & 0 deletions packages/@lwc/engine-core/src/framework/rendering.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import { patchProps } from './modules/props';
import { patchClassAttribute } from './modules/computed-class-attr';
import { patchStyleAttribute } from './modules/computed-style-attr';
import { applyEventListeners } from './modules/events';
import { patchDynamicEventListeners } from './modules/dynamic-events';
import { applyStaticClassAttribute } from './modules/static-class-attr';
import { applyStaticStyleAttribute } from './modules/static-style-attr';
import { applyRefs } from './modules/refs';
Expand Down Expand Up @@ -586,6 +587,7 @@ function patchElementPropsAndAttrsAndRefs(
}

const { owner } = vnode;
patchDynamicEventListeners(oldVnode, vnode, renderer, owner);
// Attrs need to be applied to element before props IE11 will wipe out value on radio inputs if
// value is set before type=radio.
patchClassAttribute(oldVnode, vnode, renderer);
Expand Down
3 changes: 3 additions & 0 deletions packages/@lwc/engine-core/src/framework/vm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,8 @@ export interface VM<N = HostNode, E = HostElement> {
readonly owner: VM<N, E> | null;
/** References to elements rendered using lwc:ref (template refs) */
refVNodes: RefVNodes | null;
/** event listeners added to elements corresponding to functions provided by lwc:on */
attachedEventListeners: WeakMap<Element, Record<string, EventListener | undefined>>;
/** Whether or not the VM was hydrated */
readonly hydrated: boolean;
/** Rendering operations associated with the VM */
Expand Down Expand Up @@ -344,6 +346,7 @@ export function createVM<HostNode, HostElement>(
mode,
owner,
refVNodes: null,
attachedEventListeners: new WeakMap(),
children: EmptyArray,
aChildren: EmptyArray,
velements: EmptyArray,
Expand Down
2 changes: 2 additions & 0 deletions packages/@lwc/engine-core/src/framework/vnodes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,8 @@ export interface VNodeData {
readonly styleDecls?: ReadonlyArray<[string, string, boolean]>;
readonly context?: Readonly<Record<string, Readonly<Record<string, any>>>>;
readonly on?: Readonly<Record<string, (event: Event) => any>>;
readonly dynamicOn?: Readonly<Record<string, (event: Event) => any>>; // clone of object passed to lwc:on, used to patch event listeners
readonly dynamicOnRaw?: Readonly<Record<string, (event: Event) => any>>; // object passed to lwc:on, used to verify whether object reference has changed
readonly svg?: boolean;
readonly renderer?: RendererAPI;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/@lwc/errors/src/compiler/error-info/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/
/**
* Next error code: 1203
* Next error code: 1207
*/

export * from './compiler';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -947,4 +947,36 @@ export const ParserDiagnostics = {
level: DiagnosticLevel.Warning,
url: '',
},

INVALID_LWC_ON_ELEMENT: {
code: 1203,
message:
'Invalid lwc:on usage on element "{0}". The directive can\'t be used on a template element.',
level: DiagnosticLevel.Error,
url: '',
},

INVALID_LWC_ON_LITERAL_PROP: {
code: 1204,
message:
'Invalid lwc:on usage on element "{0}". The directive binding must be an expression.',
level: DiagnosticLevel.Error,
url: '',
},

INVALID_LWC_ON_WITH_DECLARATIVE_LISTENERS: {
code: 1205,
message:
'Invalid lwc:on usage on element "{0}". It is not permitted to use declarative event listeners alongside lwc:on',
level: DiagnosticLevel.Error,
url: '',
},

INVALID_LWC_ON_OPTS: {
code: 1206,
message:
'Invalid lwc:on usage. The `lwc:on` directive must be enabled in order to use this feature.',
level: DiagnosticLevel.Error,
url: '',
},
};
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ function createPreprocessor(config, emitter, logger) {
strict: true,
},
enableDynamicComponents: true,
enableLwcOn: true,
experimentalComplexExpressions,
enableStaticContentOptimization: !DISABLE_STATIC_CONTENT_OPTIMIZATION,
disableSyntheticShadowSupport: DISABLE_SYNTHETIC_SHADOW_SUPPORT_IN_COMPILER,
Expand Down
Loading
Loading