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.

Sign up for GitHub

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

Jump to bottom

feat: log shadow mode through engine reporting API #3878

Merged
merged 4 commits into from
Dec 5, 2023
Merged

feat: log shadow mode through engine reporting API #3878

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
51d9e5d
feat: log shadow mode through engine reporting API
jye-sf Nov 22, 2023
79a0180
fix: aria reflection polyfill tests
jye-sf Nov 22, 2023
c64b282
feat: test util for filtering dispatched runtime events
jye-sf Nov 27, 2023
4fe18bb
fix: update tests and PR feedback
jye-sf Dec 4, 2023
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
8 changes: 8 additions & 0 deletions packages/@lwc/engine-core/src/framework/reporting.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,16 @@
*/
import { noop } from '@lwc/shared';

import { ShadowMode } from './vm';

export const enum ReportingEventId {
CrossRootAriaInSyntheticShadow = 'CrossRootAriaInSyntheticShadow',
CompilerRuntimeVersionMismatch = 'CompilerRuntimeVersionMismatch',
NonStandardAriaReflection = 'NonStandardAriaReflection',
TemplateMutation = 'TemplateMutation',
StylesheetMutation = 'StylesheetMutation',
ConnectedCallbackWhileDisconnected = 'ConnectedCallbackWhileDisconnected',
ShadowModeUsage = 'ShadowModeUsage',
}

export interface BasePayload {
Expand Down Expand Up @@ -44,13 +47,18 @@ export interface StylesheetMutationPayload extends BasePayload {

export interface ConnectedCallbackWhileDisconnectedPayload extends BasePayload {}

export interface ShadowModeUsagePayload extends BasePayload {
mode: ShadowMode;
}

export type ReportingPayloadMapping = {
[ReportingEventId.CrossRootAriaInSyntheticShadow]: CrossRootAriaInSyntheticShadowPayload;
[ReportingEventId.CompilerRuntimeVersionMismatch]: CompilerRuntimeVersionMismatchPayload;
[ReportingEventId.NonStandardAriaReflection]: NonStandardAriaReflectionPayload;
[ReportingEventId.TemplateMutation]: TemplateMutationPayload;
[ReportingEventId.StylesheetMutation]: StylesheetMutationPayload;
[ReportingEventId.ConnectedCallbackWhileDisconnected]: ConnectedCallbackWhileDisconnectedPayload;
[ReportingEventId.ShadowModeUsage]: ShadowModeUsagePayload;
};

export type ReportingDispatcher<T extends ReportingEventId = ReportingEventId> = (
Expand Down
7 changes: 7 additions & 0 deletions packages/@lwc/engine-core/src/framework/vm.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,13 @@ export function createVM<HostNode, HostElement>(
vm.shadowMode = computeShadowMode(def, vm.owner, renderer);
vm.tro = getTemplateReactiveObserver(vm);

if (isReportingEnabled()) {
report(ReportingEventId.ShadowModeUsage, {
tagName: vm.tagName,
mode: vm.shadowMode,
});
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked, and this gets tree-shaken if the customer is not using the reporting API! 🎉

if (process.env.NODE_ENV !== 'production') {
vm.toString = (): string => {
return `[object:vm ${def.name} (${vm.idx})]`;
Expand Down
22 changes: 14 additions & 8 deletions packages/@lwc/integration-karma/test/accessibility/non-standard-aria-props/index.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ if (process.env.ENABLE_ARIA_REFLECTION_GLOBAL_POLYFILL) {
elm.getProp(prop);
}).not.toLogWarningDev();

expect(dispatcher).not.toHaveBeenCalled();
expect(dispatcher).not.toHaveBeenCalledWith(
'NonStandardAriaReflection'
);
jye-sf marked this conversation as resolved.
Show resolved Hide resolved
});

it('LightningElement - outside component', () => {
Expand All @@ -52,7 +54,9 @@ if (process.env.ENABLE_ARIA_REFLECTION_GLOBAL_POLYFILL) {
return unused; // remove lint warning
}).not.toLogWarningDev();

expect(dispatcher).not.toHaveBeenCalled();
expect(dispatcher).not.toHaveBeenCalledWith(
'NonStandardAriaReflection'
);
});

it('regular Element inside LightningElement', () => {
Expand All @@ -61,8 +65,7 @@ if (process.env.ENABLE_ARIA_REFLECTION_GLOBAL_POLYFILL) {
elm.getPropOnElement(prop);
}).toLogWarningDev(inComponentWarning);

expect(dispatcher).toHaveBeenCalledTimes(2);
expect(dispatcher.calls.allArgs()).toEqual([
[
[
'NonStandardAriaReflection',
{
Expand All @@ -81,7 +84,9 @@ if (process.env.ENABLE_ARIA_REFLECTION_GLOBAL_POLYFILL) {
setValueType: undefined,
},
],
]);
].forEach((dispatch) => {
expect(dispatcher.calls.allArgs()).toContain(dispatch);
});
});

it('regular Element outside LightningElement', () => {
Expand All @@ -93,8 +98,7 @@ if (process.env.ENABLE_ARIA_REFLECTION_GLOBAL_POLYFILL) {
return unused; // remove lint warning
}).toLogWarningDev(outsideComponentWarning);

expect(dispatcher).toHaveBeenCalledTimes(2);
expect(dispatcher.calls.allArgs()).toEqual([
[
[
'NonStandardAriaReflection',
{
Expand All @@ -113,7 +117,9 @@ if (process.env.ENABLE_ARIA_REFLECTION_GLOBAL_POLYFILL) {
setValueType: undefined,
},
],
]);
].forEach((dispatch) => {
expect(dispatcher.calls.allArgs()).toContain(dispatch);
});
});
});
});
Expand Down
151 changes: 80 additions & 71 deletions packages/@lwc/integration-karma/test/accessibility/synthetic-cross-root-aria/index.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,18 +95,19 @@ if (!process.env.NATIVE_SHADOW) {
expect(() => {
elm.linkUsingAriaLabelledBy();
}).toLogWarningDev(expectedMessages);
expect(dispatcher.calls.allArgs()).toEqual(
getExpectedDispatcherCalls(true)
);

getExpectedDispatcherCalls(true).forEach((dispatch) => {
expect(dispatcher.calls.allArgs()).toContain(dispatch);
});
});

it('setting id', () => {
expect(() => {
elm.linkUsingId();
}).toLogWarningDev(expectedMessages);
expect(dispatcher.calls.allArgs()).toEqual(
getExpectedDispatcherCalls(false)
);
getExpectedDispatcherCalls(false).forEach((dispatch) => {
expect(dispatcher.calls.allArgs()).toContain(dispatch);
});
});

it('linking to an element in global light DOM', () => {
Expand All @@ -116,9 +117,9 @@ if (!process.env.NATIVE_SHADOW) {
expect(() => {
sourceElm.setAriaLabelledBy('foo');
}).toLogWarningDev(expectedMessages);
expect(dispatcher.calls.allArgs()).toEqual(
getExpectedDispatcherCalls(true)
);
getExpectedDispatcherCalls(true).forEach((dispatch) => {
expect(dispatcher.calls.allArgs()).toContain(dispatch);
});
});

it('linking from an element in global light DOM', () => {
Expand All @@ -129,65 +130,69 @@ if (!process.env.NATIVE_SHADOW) {
expect(() => {
targetElm.setId('foo');
}).toLogWarningDev(expectedMessageForCrossRootWithTargetAsVM);
expect(dispatcher.calls.allArgs()).toEqual([
[
'CrossRootAriaInSyntheticShadow',
{
tagName: 'x-aria-target',
attributeName: 'aria-labelledby',
},
],
expect(dispatcher.calls.allArgs()).toContain([
'CrossRootAriaInSyntheticShadow',
{
tagName: 'x-aria-target',
attributeName: 'aria-labelledby',
},
]);
});

[null, '', ' '].forEach((value) => {
it(`ignores setting id to ${JSON.stringify(value)}`, () => {
targetElm.setId(value);
expect(dispatcher).not.toHaveBeenCalled();
expect(dispatcher).not.toHaveBeenCalledWith(
'CrossRootAriaInSyntheticShadow'
);
expect(dispatcher).not.toHaveBeenCalledWith(
'NonStandardAriaReflection'
);
});

it(`ignores setting aria-labelledby to ${JSON.stringify(value)}`, () => {
expectWarningForNonStandardPropertyAccess(() => {
sourceElm.setAriaLabelledBy(value);
});
expect(dispatcher.calls.allArgs()).toEqual([
...(usePropertyAccess
? [
[
'NonStandardAriaReflection',
{
tagName: 'x-aria-source',
propertyName: 'ariaLabelledBy',
isSetter: true,
setValueType:
value === null ? 'null' : typeof value,
},
],
]
: []),
]);

const dispatch = [
'NonStandardAriaReflection',
{
tagName: 'x-aria-source',
propertyName: 'ariaLabelledBy',
isSetter: true,
setValueType: value === null ? 'null' : typeof value,
},
];

if (usePropertyAccess) {
expect(dispatcher.calls.allArgs()).toContain(dispatch);
} else {
expect(dispatcher.calls.allArgs()).not.toContain(dispatch);
}
});
});

it('ignores id that references nonexistent element', () => {
expectWarningForNonStandardPropertyAccess(() => {
sourceElm.setAriaLabelledBy('does-not-exist-at-all-lol');
});
expect(dispatcher.calls.allArgs()).toEqual([
...(usePropertyAccess
? [
[
'NonStandardAriaReflection',
{
tagName: 'x-aria-source',
propertyName: 'ariaLabelledBy',
isSetter: true,
setValueType: 'string',
},
],
]
: []),
]);

const dispatch = [
'NonStandardAriaReflection',
{
tagName: 'x-aria-source',
propertyName: 'ariaLabelledBy',
isSetter: true,
setValueType: 'string',
},
];

if (usePropertyAccess) {
expect(dispatcher.calls.allArgs()).toContain(dispatch);
} else {
expect(dispatcher.calls.allArgs()).not.toContain(dispatch);
}
});

[
Expand All @@ -203,9 +208,9 @@ if (!process.env.NATIVE_SHADOW) {
expect(() => {
elm.linkUsingBoth(options);
}).toLogWarningDev(expectedMessages);
expect(dispatcher.calls.allArgs()).toEqual(
getExpectedDispatcherCalls(true)
);
getExpectedDispatcherCalls(true).forEach((dispatch) => {
expect(dispatcher.calls.allArgs()).toContain(dispatch);
});
});

it('linking multiple targets', () => {
Expand All @@ -215,7 +220,8 @@ if (!process.env.NATIVE_SHADOW) {
...expectedMessages,
expectedMessageForCrossRootForSecondTarget,
]);
expect(dispatcher.calls.allArgs()).toEqual([

[
...getExpectedDispatcherCalls(true),
[
'CrossRootAriaInSyntheticShadow',
Expand All @@ -224,7 +230,9 @@ if (!process.env.NATIVE_SHADOW) {
attributeName: 'aria-labelledby',
},
],
]);
].forEach((dispatch) => {
expect(dispatcher.calls.allArgs()).toContain(dispatch);
});
});
});
});
Expand All @@ -245,22 +253,16 @@ if (!process.env.NATIVE_SHADOW) {
}).toLogWarningDev(expectedMessageForCrossRoot);

// dispatcher is still called twice
expect(dispatcher.calls.allArgs()).toEqual([
[
'CrossRootAriaInSyntheticShadow',
{
tagName: 'x-aria-source',
attributeName: 'aria-labelledby',
},
],
[
'CrossRootAriaInSyntheticShadow',
{
tagName: 'x-aria-source',
attributeName: 'aria-labelledby',
},
],
]);
expect(
dispatcher.calls
.allArgs()
.filter(
(dispatch) =>
dispatch[0] === 'CrossRootAriaInSyntheticShadow' &&
dispatch[1].tagName === 'x-aria-source' &&
dispatch[1].attributeName === 'aria-labelledby'
)
).toHaveSize(2);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is less clear than the previous (equivalent) code. I would prefer to change it back, but I also don't have a really strong opinion, since it's just test code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This variation is less strict about whether the dispatcher includes calls for other events. The previous code broke with the addition of new reporting events, while this one would not.

I'm fine changing it back since it's less critical to have that distinction with the new helper function. The previous behavior would still be asserting that the dispatcher is called twice for CrossRootAriaInSyntheticShadow AND zero times for NonStandardAriaReflection, but that's behavior less likely to break in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep that's exactly what I was thinking – let's use the new attachReportingControlDispatcher to control the filtering, rather than doing it here.

});

[false, true].forEach((reverseOrder) => {
Expand All @@ -269,7 +271,14 @@ if (!process.env.NATIVE_SHADOW) {
const valid = createElement('x-valid', { is: Valid });
document.body.appendChild(valid);
valid.linkElements({ reverseOrder });
expect(dispatcher).not.toHaveBeenCalled();

expect(dispatcher).not.toContain(
jasmine.arrayContaining('CrossRootAriaInSyntheticShadow')
);

expect(dispatcher).not.toContain(
jasmine.arrayContaining('NonStandardAriaReflection')
);
});
});
});
Expand Down
9 changes: 5 additions & 4 deletions packages/@lwc/integration-karma/test/api/freezeTemplate/index.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ describe('freezeTemplate', () => {
expect(descriptor.enumerable).toEqual(true);
expect(descriptor.configurable).toEqual(true);
}
expect(dispatcher).not.toHaveBeenCalled();
expect(dispatcher).not.toHaveBeenCalledWith('StylesheetMutation');
});

describe('ENABLE_FROZEN_TEMPLATE set to true', () => {
Expand All @@ -249,7 +249,8 @@ describe('freezeTemplate', () => {

expect(Object.isFrozen(template)).toEqual(true);
expect(Object.isFrozen(template.stylesheets)).toEqual(true);
expect(dispatcher).not.toHaveBeenCalled();
expect(dispatcher).not.toHaveBeenCalledWith('StylesheetMutation');
expect(dispatcher).not.toHaveBeenCalledWith('TemplateMutation');
});

it('freezes a template with no stylesheets', () => {
Expand All @@ -258,7 +259,7 @@ describe('freezeTemplate', () => {

expect(Object.isFrozen(template)).toEqual(true);
expect(template.stylesheets).toEqual(undefined);
expect(dispatcher).not.toHaveBeenCalled();
expect(dispatcher).not.toHaveBeenCalledWith('TemplateMutation');
});

it('deep-freezes the stylesheets', () => {
Expand All @@ -275,7 +276,7 @@ describe('freezeTemplate', () => {
expect(Object.isFrozen(stylesheets[0])).toEqual(true);
expect(Object.isFrozen(stylesheets[1])).toEqual(true);
expect(Object.isFrozen(stylesheets[1][0])).toEqual(true);
expect(dispatcher).not.toHaveBeenCalled();
expect(dispatcher).not.toHaveBeenCalledWith('StylesheetMutation');
});
});
});
Loading