Skip to content
Closed
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
9 changes: 6 additions & 3 deletions src/Components/Web.JS/src/Rendering/Events/EventTypes.ts
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've been looking more at this. And we shouldn't change this to a warning.

There was already an issue we fixed with the event being dispatched twice. Now we are converting something deterministic and invalid into something "valid" that produces nondeterministic behavior (first registration wins). And the only way to find it is if you happen to look at the console errors, which is not something we can generally rely on people doing.

Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,13 @@ export function registerCustomEventType(eventName: string, options: EventTypeOpt
throw new Error('The options parameter is required.');
}

// There can't be more than one registration for the same event name because then we wouldn't
// know which eventargs data to supply.
// Duplicate registrations can occur legitimately when JS initializers (lib.module.js)
// re-execute after enhanced navigation or circuit reconnection.
if (eventTypeRegistry.has(eventName)) {
throw new Error(`The event '${eventName}' is already registered.`);
console.warn(`The event '${eventName}' is being registered more than once. ` +
'Duplicate registrations will be ignored. ' +
'This may indicate that a JS initializer is running multiple times.');
Comment on lines +23 to +25
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we have an estimate on how often would you see this in the legitimate cases? Just so we don't spam the console so much that people would view it as the app being buggy.

return;
}

// When aliasing a browser event, the custom event name must be different from the browser event name
Expand Down
127 changes: 127 additions & 0 deletions src/Components/Web.JS/test/EventTypes.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
import { expect, test, describe, afterEach, jest } from '@jest/globals';
import { registerCustomEventType, getEventTypeOptions, getEventNameAliases } from '../src/Rendering/Events/EventTypes';

// The eventTypeRegistry is module-scoped and persists across tests, so we need
// unique event names per test to avoid interference from built-in registrations
// and other tests.

describe('registerCustomEventType duplicate registration', () => {
afterEach(() => {
jest.restoreAllMocks();
});

test('duplicate registration with same browserEventName warns and is ignored', () => {
const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});

const options1 = {
Comment thread
ilonatommy marked this conversation as resolved.
browserEventName: 'change',
createEventArgs: (e: Event) => ({ checked: true }),
};

const options2 = {
browserEventName: 'change',
createEventArgs: (e: Event) => ({ checked: false, extra: 'data' }),
};

// First registration succeeds
registerCustomEventType('mycheckedchange', options1);

// Second registration with same browserEventName — should NOT throw
registerCustomEventType('mycheckedchange', options2);

// Should have warned
expect(warnSpy).toHaveBeenCalledWith(
expect.stringContaining('mycheckedchange')
);

// First registration wins (the registry entry is not overwritten)
const registered = getEventTypeOptions('mycheckedchange');
expect(registered).toBe(options1);

// The alias array should only have one entry, not two
const aliases = getEventNameAliases('change');
const count = aliases!.filter(a => a === 'mycheckedchange').length;
expect(count).toBe(1);
});

test('duplicate registration with different browserEventName warns and is ignored', () => {
const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});

registerCustomEventType('myevent-diffbrowser', {
browserEventName: 'click',
createEventArgs: (e: Event) => ({}),
});

// Different browserEventName — still should NOT throw, just warn
registerCustomEventType('myevent-diffbrowser', {
browserEventName: 'mouseover',
createEventArgs: (e: Event) => ({}),
});

expect(warnSpy).toHaveBeenCalled();

// First registration wins
const registered = getEventTypeOptions('myevent-diffbrowser');
expect(registered!.browserEventName).toBe('click');
});

test('duplicate registration does not cause double alias entries', () => {
jest.spyOn(console, 'warn').mockImplementation(() => {});

registerCustomEventType('mytabchange', {
browserEventName: 'change',
createEventArgs: (e: Event) => ({ activeId: 'tab1' }),
});

// Register again — simulates JS initializer re-firing
registerCustomEventType('mytabchange', {
browserEventName: 'change',
createEventArgs: (e: Event) => ({ activeId: 'tab2' }),
});

// Register a third time
registerCustomEventType('mytabchange', {
browserEventName: 'change',
createEventArgs: (e: Event) => ({ activeId: 'tab3' }),
});

// The alias for 'change' should only contain 'mytabchange' once
const aliases = getEventNameAliases('change');
const count = aliases!.filter(a => a === 'mytabchange').length;
expect(count).toBe(1);
});

test('simulates FluentUI re-initialization scenario', () => {
const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});

const fluentEvents = [
{ name: 'flu-checkedchange', browserEventName: 'change' },
{ name: 'flu-switchcheckedchange', browserEventName: 'change' },
{ name: 'flu-sliderchange', browserEventName: 'change' },
{ name: 'flu-accordionchange', browserEventName: 'change' },
{ name: 'flu-tabchange', browserEventName: 'change' },
];

// First initialization — all should register successfully
for (const evt of fluentEvents) {
registerCustomEventType(evt.name, {
browserEventName: evt.browserEventName,
createEventArgs: (e: Event) => ({}),
});
}

expect(warnSpy).not.toHaveBeenCalled();

// Second initialization (simulates enhanced nav / circuit reconnect)
// — should NOT throw, just warn
for (const evt of fluentEvents) {
registerCustomEventType(evt.name, {
browserEventName: evt.browserEventName,
createEventArgs: (e: Event) => ({}),
});
}

// Should have warned for each duplicate
expect(warnSpy).toHaveBeenCalledTimes(fluentEvents.length);
});
});
Loading