Skip to content

Commit

Permalink
Add warning when using popoverTarget={Element}
Browse files Browse the repository at this point in the history
  • Loading branch information
eps1lon committed May 20, 2024
1 parent f74fb57 commit 0850281
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 6 deletions.
8 changes: 4 additions & 4 deletions fixtures/attribute-behavior/AttributeTableSnapshot.md
Original file line number Diff line number Diff line change
Expand Up @@ -8476,11 +8476,11 @@
## `popoverTarget` (on `<button>` inside `<div>`)
| Test Case | Flags | Result |
| --- | --- | --- |
| `popoverTarget=(string)`| (initial)| `<null>` |
| `popoverTarget=(string)`| (changed)| `<HTMLDivElement>` |
| `popoverTarget=(empty string)`| (initial)| `<null>` |
| `popoverTarget=(array with string)`| (initial)| `<null>` |
| `popoverTarget=(empty array)`| (initial)| `<null>` |
| `popoverTarget=(object)`| (initial)| `<null>` |
| `popoverTarget=(array with string)`| (changed, warning, ssr warning)| `<HTMLDivElement>` |
| `popoverTarget=(empty array)`| (initial, warning, ssr warning)| `<null>` |
| `popoverTarget=(object)`| (initial, warning, ssr warning)| `<null>` |
| `popoverTarget=(numeric string)`| (initial)| `<null>` |
| `popoverTarget=(-1)`| (initial)| `<null>` |
| `popoverTarget=(0)`| (initial)| `<null>` |
Expand Down
1 change: 1 addition & 0 deletions fixtures/attribute-behavior/public/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
You need to enable JavaScript to run this app.
</noscript>
<div id="root"></div>
<div id="popover-target" popover="auto"></div>
<!--
This HTML file is a template.
If you open it directly in the browser, you will see an empty page.
Expand Down
11 changes: 10 additions & 1 deletion fixtures/attribute-behavior/src/attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -1450,7 +1450,16 @@ const attributes = [
{name: 'popover', overrideStringValue: 'manual'},
{
name: 'popoverTarget',
read: element => element.popoverTargetElement,
read: element => {
document.body.appendChild(element);
try {
// trigger and target need to be connected for `popoverTargetElement` to read the actual value.
return element.popoverTargetElement;
} finally {
document.body.removeChild(element);
}
},
overrideStringValue: 'popover-target',
tagName: 'button',
},
{name: 'popoverTargetAction', overrideStringValue: 'show', tagName: 'button'},
Expand Down
15 changes: 15 additions & 0 deletions packages/react-dom-bindings/src/client/ReactDOMComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ let didWarnFormActionName = false;
let didWarnFormActionTarget = false;
let didWarnFormActionMethod = false;
let didWarnForNewBooleanPropsWithEmptyValue: {[string]: boolean};
let didWarnPopoverTargetObject = false;
let canDiffStyleForHydrationWarning;
if (__DEV__) {
didWarnForNewBooleanPropsWithEmptyValue = {};
Expand Down Expand Up @@ -866,6 +867,20 @@ function setProp(
case 'innerText':
case 'textContent':
break;
case 'popoverTarget':
if (__DEV__) {
if (
!didWarnPopoverTargetObject &&
value != null &&
typeof value === 'object'
) {
didWarnPopoverTargetObject = true;
console.error(
'The `popoverTarget` prop expects the ID of an Element as a string. Received %s instead.',
value,
);
}
}
// Fall through
default: {
if (
Expand Down
30 changes: 29 additions & 1 deletion packages/react-dom/src/__tests__/DOMPropertyOperations-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,13 @@ describe('DOMPropertyOperations', () => {
let React;
let ReactDOMClient;
let act;
let assertConsoleErrorDev;

beforeEach(() => {
jest.resetModules();
React = require('react');
ReactDOMClient = require('react-dom/client');
({act} = require('internal-test-utils'));
({act, assertConsoleErrorDev} = require('internal-test-utils'));
});

// Sets a value in a way that React doesn't see,
Expand Down Expand Up @@ -1317,6 +1318,33 @@ describe('DOMPropertyOperations', () => {
});
expect(customElement.foo).toBe(undefined);
});

it('warns when using popoverTarget={HTMLElement}', async () => {
const popoverTarget = document.createElement('div');
const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);

await act(() => {
root.render(
<button key="one" popoverTarget={popoverTarget}>
Toggle popover
</button>,
);
});

assertConsoleErrorDev([
'The `popoverTarget` prop expects the ID of an Element as a string. Received [object HTMLDivElement] instead.',
]);

// Dedupe warning
await act(() => {
root.render(
<button key="two" popoverTarget={popoverTarget}>
Toggle popover
</button>,
);
});
});
});

describe('deleteValueForProperty', () => {
Expand Down

0 comments on commit 0850281

Please sign in to comment.