Skip to content

Commit

Permalink
Forbid createEventHandle with custom events
Browse files Browse the repository at this point in the history
We can't support this without adding more complexity. It's not clear that this is even desirable, as none of our existing use cases need custom events. This API primarily exists as a deprecation strategy for Flare, so I don't think it is important to expand its support beyond what Flare replacement code currently needs. We can later revisit it with a better understanding of the eager/lazy tradeoff but for now let's remove the inconsistency.
  • Loading branch information
gaearon committed Aug 24, 2020
1 parent 49216d6 commit 9bf255b
Show file tree
Hide file tree
Showing 4 changed files with 164 additions and 70 deletions.
22 changes: 22 additions & 0 deletions packages/react-dom/src/client/ReactDOMEventHandle.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import type {
} from '../shared/ReactDOMTypes';

import {getEventPriorityForListenerSystem} from '../events/DOMEventProperties';
import {allNativeEvents} from '../events/EventRegistry';
import {
getClosestInstanceFromNode,
getEventHandlerListeners,
Expand All @@ -33,6 +34,7 @@ import {IS_EVENT_HANDLE_NON_MANAGED_NODE} from '../events/EventSystemFlags';
import {
enableScopeAPI,
enableCreateEventHandleAPI,
enableEagerRootListeners,
} from 'shared/ReactFeatureFlags';
import invariant from 'shared/invariant';

Expand Down Expand Up @@ -178,6 +180,26 @@ export function createEventHandle(
): ReactDOMEventHandle {
if (enableCreateEventHandleAPI) {
const domEventName = ((type: any): DOMEventName);

if (enableEagerRootListeners) {
// We cannot support arbitrary native events with eager root listeners
// because the eager strategy relies on knowing the whole list ahead of time.
// If we wanted to support this, we'd have to add code to keep track
// (or search) for all portal and root containers, and lazily add listeners
// to them whenever we see a previously unknown event. This seems like a lot
// of complexity for something we don't even have a particular use case for.
// Unfortunately, the downside of this invariant is that *removing* a native
// event from the list of known events has now become a breaking change for
// any code relying on the createEventHandle API.
invariant(
allNativeEvents.has(domEventName) ||
domEventName === 'beforeblur' ||
domEventName === 'afterblur',
'Cannot call unstable_createEventHandle with "%s", as it is not an event known to React.',
domEventName,
);
}

let isCapturePhaseListener = false;
let isPassiveListener = undefined; // Undefined means to use the browser default
let listenerPriority;
Expand Down
3 changes: 2 additions & 1 deletion packages/react-dom/src/events/DOMEventProperties.js
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,9 @@ export function getEventPriorityForListenerSystem(
}
if (__DEV__) {
console.warn(
'The event "type" provided to createEventHandle() does not have a known priority type.' +
'The event "%s" provided to createEventHandle() does not have a known priority type.' +
' It is recommended to provide a "priority" option to specify a priority.',
type,
);
}
return ContinuousEvent;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2401,85 +2401,97 @@ describe('DOMPluginEventSystem', () => {
);

let setCustomEventHandle;
if (gate(flags => flags.enableEagerRootListeners)) {
// With eager listeners, supporting custom events via this API doesn't make sense
// because we can't know a full list of them ahead of time. Let's check we throw
// since otherwise we'd end up with inconsistent behavior, like no portal bubbling.
expect(() => {
setCustomEventHandle = ReactDOM.unstable_createEventHandle(
'custom-event',
);
}).toThrow(
'Cannot call unstable_createEventHandle with "custom-event", as it is not an event known to React.',
);
} else {
// Test that we get a warning when we don't provide an explicit priority
expect(() => {
setCustomEventHandle = ReactDOM.unstable_createEventHandle(
'custom-event',
);
}).toWarnDev(
'Warning: The event "custom-event" provided to createEventHandle() does not have a known priority type. ' +
'It is recommended to provide a "priority" option to specify a priority.',
{withoutStack: true},
);

// Test that we get a warning when we don't provide an explicit priority
expect(() => {
setCustomEventHandle = ReactDOM.unstable_createEventHandle(
'custom-event',
{
priority: 0, // Discrete
},
);
}).toWarnDev(
'Warning: The event "type" provided to createEventHandle() does not have a known priority type. ' +
'It is recommended to provide a "priority" option to specify a priority.',
{withoutStack: true},
);

setCustomEventHandle = ReactDOM.unstable_createEventHandle(
'custom-event',
{
priority: 0, // Discrete
},
);

const setCustomCaptureHandle = ReactDOM.unstable_createEventHandle(
'custom-event',
{
capture: true,
priority: 0, // Discrete
},
);
const setCustomCaptureHandle = ReactDOM.unstable_createEventHandle(
'custom-event',
{
capture: true,
priority: 0, // Discrete
},
);

function Test() {
React.useEffect(() => {
const clearCustom1 = setCustomEventHandle(
buttonRef.current,
onCustomEvent,
);
const clearCustom2 = setCustomCaptureHandle(
buttonRef.current,
onCustomEventCapture,
);
const clearCustom3 = setCustomEventHandle(
divRef.current,
onCustomEvent,
);
const clearCustom4 = setCustomCaptureHandle(
divRef.current,
onCustomEventCapture,
);
const Test = () => {
React.useEffect(() => {
const clearCustom1 = setCustomEventHandle(
buttonRef.current,
onCustomEvent,
);
const clearCustom2 = setCustomCaptureHandle(
buttonRef.current,
onCustomEventCapture,
);
const clearCustom3 = setCustomEventHandle(
divRef.current,
onCustomEvent,
);
const clearCustom4 = setCustomCaptureHandle(
divRef.current,
onCustomEventCapture,
);

return () => {
clearCustom1();
clearCustom2();
clearCustom3();
clearCustom4();
};
});
return () => {
clearCustom1();
clearCustom2();
clearCustom3();
clearCustom4();
};
});

return (
<button ref={buttonRef}>
<div ref={divRef}>Click me!</div>
</button>
);
}
return (
<button ref={buttonRef}>
<div ref={divRef}>Click me!</div>
</button>
);
};

ReactDOM.render(<Test />, container);
Scheduler.unstable_flushAll();
ReactDOM.render(<Test />, container);
Scheduler.unstable_flushAll();

const buttonElement = buttonRef.current;
dispatchEvent(buttonElement, 'custom-event');
expect(onCustomEvent).toHaveBeenCalledTimes(1);
expect(onCustomEventCapture).toHaveBeenCalledTimes(1);
expect(log[0]).toEqual(['capture', buttonElement]);
expect(log[1]).toEqual(['bubble', buttonElement]);
const buttonElement = buttonRef.current;
dispatchEvent(buttonElement, 'custom-event');
expect(onCustomEvent).toHaveBeenCalledTimes(1);
expect(onCustomEventCapture).toHaveBeenCalledTimes(1);
expect(log[0]).toEqual(['capture', buttonElement]);
expect(log[1]).toEqual(['bubble', buttonElement]);

const divElement = divRef.current;
dispatchEvent(divElement, 'custom-event');
expect(onCustomEvent).toHaveBeenCalledTimes(3);
expect(onCustomEventCapture).toHaveBeenCalledTimes(3);
expect(log[2]).toEqual(['capture', buttonElement]);
expect(log[3]).toEqual(['capture', divElement]);
expect(log[4]).toEqual(['bubble', divElement]);
expect(log[5]).toEqual(['bubble', buttonElement]);
const divElement = divRef.current;
dispatchEvent(divElement, 'custom-event');
expect(onCustomEvent).toHaveBeenCalledTimes(3);
expect(onCustomEventCapture).toHaveBeenCalledTimes(3);
expect(log[2]).toEqual(['capture', buttonElement]);
expect(log[3]).toEqual(['capture', divElement]);
expect(log[4]).toEqual(['bubble', divElement]);
expect(log[5]).toEqual(['bubble', buttonElement]);
}
});

// @gate experimental
Expand Down Expand Up @@ -2823,6 +2835,64 @@ describe('DOMPluginEventSystem', () => {
expect(log[5]).toEqual(['bubble', buttonElement]);
});

// @gate experimental && enableEagerRootListeners
it('propagates known createEventHandle events through portals without inner listeners', () => {
const buttonRef = React.createRef();
const divRef = React.createRef();
const log = [];
const onClick = jest.fn(e => log.push(['bubble', e.currentTarget]));
const onClickCapture = jest.fn(e =>
log.push(['capture', e.currentTarget]),
);
const setClick = ReactDOM.unstable_createEventHandle('click');
const setClickCapture = ReactDOM.unstable_createEventHandle(
'click',
{
capture: true,
},
);

const portalElement = document.createElement('div');
document.body.appendChild(portalElement);

function Child() {
return <div ref={divRef}>Click me!</div>;
}

function Parent() {
React.useEffect(() => {
const clear1 = setClick(buttonRef.current, onClick);
const clear2 = setClickCapture(
buttonRef.current,
onClickCapture,
);
return () => {
clear1();
clear2();
};
});

return (
<button ref={buttonRef}>
{ReactDOM.createPortal(<Child />, portalElement)}
</button>
);
}

ReactDOM.render(<Parent />, container);
Scheduler.unstable_flushAll();

const divElement = divRef.current;
const buttonElement = buttonRef.current;
dispatchClickEvent(divElement);
expect(onClick).toHaveBeenCalledTimes(1);
expect(onClickCapture).toHaveBeenCalledTimes(1);
expect(log[0]).toEqual(['capture', buttonElement]);
expect(log[1]).toEqual(['bubble', buttonElement]);

document.body.removeChild(portalElement);
});

describe('Compatibility with Scopes API', () => {
beforeEach(() => {
jest.resetModules();
Expand Down
3 changes: 2 additions & 1 deletion scripts/error-codes/codes.json
Original file line number Diff line number Diff line change
Expand Up @@ -360,5 +360,6 @@
"368": "ReactDOM.createEventHandle: setListener called on an invalid target. Provide a valid EventTarget or an element managed by React.",
"369": "ReactDOM.createEventHandle: setter called on an invalid target. Provide a valid EventTarget or an element managed by React.",
"370": "ReactDOM.createEventHandle: setter called with an invalid callback. The callback must be a function.",
"371": "Text string must be rendered within a <Text> component.\n\nText: %s"
"371": "Text string must be rendered within a <Text> component.\n\nText: %s",
"372": "Cannot call unstable_createEventHandle with \"%s\", as it is not an event known to React."
}

0 comments on commit 9bf255b

Please sign in to comment.