react-admin icon indicating copy to clipboard operation
react-admin copied to clipboard

react admin error with Menu.Item missing properties

Open charleschouette opened this issue 11 months ago • 9 comments

I got an error when using the component Menu.Item of react admin. It seems that all the properties are missing. Therefore I can't build my app without adding all the 250+ properties of Menu.Item. I noticed that it works when using the component MenuItemLink. You can reproduce it by using the documentation example of Menu.Item. I am using node version v21.6.1 and npm version 10.2.4

it does not build with the code:

// in src/MyMenu.js
import { Menu } from 'react-admin';

export const MyMenu = () => (
    <Menu>
        ...
        <Menu.Item to="/custom-route" primaryText="Miscellaneous" />
    </Menu>
);
  • React-admin version: 4.16.13
  • React version:18.2.0
  • Stack trace:
error TS2739: Type '{ to: string; primaryText: string; }' is missing the following properties from type 'Pick<MenuItemLinkProps, "children" | "replace" | "value" | "id" | "className" | "dense" | "title" | "defaultChecked" | "defaultValue" | "suppressContentEditableWarning" | ... 277 more ... | "tooltipProps">': onPointerEnterCapture, onPointerLeaveCapture

it seems related to this issue: https://github.com/marmelab/react-admin/issues/1196 that has already been resolved but adding @types/react-router and @types/react-router-dom did not solve the problem.

charleschouette avatar Mar 20 '24 11:03 charleschouette

Thanks for your report.

I can somehow reproduce it in the simple example (https://stackblitz.com/edit/github-1ymznm?file=src%2FLayout.tsx), but not in the monorepo. There must be a regression somewhere in one of the dependencies.

fzaninotto avatar Mar 20 '24 12:03 fzaninotto

The two missing props seem to be onPointerEnterCapture and onPointerLeaveCapture

fzaninotto avatar Mar 20 '24 12:03 fzaninotto

The problem comes from @types/react.

  • It disappears if you downgrade to 18.2.65.
  • It reappears if you use 18.2.66.
  • It's still present in the latest version (18.2.67).

fzaninotto avatar Mar 20 '24 12:03 fzaninotto

This is indeed a problem with DefinitelyTyped: https://github.com/DefinitelyTyped/DefinitelyTyped/discussions/69006

The fix has been merged and should be released soon: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/69005

In the meantime, please limit the @types/react version in your packages.json.

fzaninotto avatar Mar 20 '24 12:03 fzaninotto

The fix has been merged and should be released soon: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/69005

I think this is not enough, as it only reverts the type removal for v16 and v17 but does not changes anything about v18: https://github.com/DefinitelyTyped/DefinitelyTyped/commit/1bfbd3bf760321d305a53f17ccff2fe98bdfb3f5

The error still appears with the latest v18.2.70 released today.

Is this solved with the next bugfix release of react-admin v4? Or will it only work with react-admin v5, which includes the react v18 update?

Pharb avatar Mar 25 '24 12:03 Pharb

react-admin can't fix this regression, as it comes from DefinitelyTyped. I suggest you downgrade to 18.2.65, and report the fact that the bug is still present in the DefinitelyTyped repo.

fzaninotto avatar Mar 26 '24 15:03 fzaninotto

The problem seems to be still present in @types/react 18.2.78 and 18.3.1.

fzaninotto avatar May 03 '24 22:05 fzaninotto

@fzaninotto , unfortunately it looks like there was a "fix" that was subsequently abandoned. Given that, and the attitude of the devs working on the @types/react, it seems very unlikely this will be "fixed" -> they said it caused too many issues so they reverted. I think the safe bet would be to remove it.

AntonOfTheWoods avatar May 07 '24 09:05 AntonOfTheWoods

@AntonOfTheWoods Can you elaborate on the attitude of the devs? Also, what would you want to "remove"?

fzaninotto avatar May 07 '24 11:05 fzaninotto

So accoding to https://github.com/DefinitelyTyped/DefinitelyTyped/pull/69243#pullrequestreview-1973380929, the React types won't be updated for React 18, so we need to fix it in react-admin.

fzaninotto avatar May 13 '24 11:05 fzaninotto

I can't reproduce the problem anymore with React 18 (in react-admin branch next). It seems to be fixed upstream.

If you still have the bug, can you give a repro? Note that React 18 isn't officially supported by react-admin v4.

fzaninotto avatar May 13 '24 11:05 fzaninotto

@fzaninotto Fix in react-admin next is great news, but what about current fresh installs? They all are partially broken type-wise, right?

create-react-admin sets up a project with react 18 - everyone assumes it is supported (more - it is recommended). Also there is no big red note about it being not officially supported anywhere in the docs, that I noticed)

It's not only MenuLinkItem affected, same behavior with InspectorButton for example

ilia-os avatar May 13 '24 15:05 ilia-os

Apologies, my mind is confused by switching back and forth between versions.

You're right, react-admin v4, the current version, does support React 18. So we need to fix this issue in the master branch.

The fact that the issue is (apparently) already fixed in the next branch isn't enough.

fzaninotto avatar May 13 '24 15:05 fzaninotto

I am willing to help, yet i'm not sure what the fix would be. At least i'll confirm it will resolve) I've found this issue from ant-design though https://github.com/ant-design/ant-design-icons/pull/631 - they removed all forwardRef calls

Also, InspectorButton requires one additinal prop "placeholder", which seems useless. Its being passed to mui IconButton which uses forwardRef But ResourceMenuItem, which relies on MenuItemLink (which also uses forwardRef) does not have this issue and works fine... Maybe we could provide "safe" variants without top-level forwardRef + update unsafe components jsdoc, or just overwrite the component types..

image

ilia-os avatar May 13 '24 16:05 ilia-os

It would be a great gelp if you could list here the components that trigger the compilation error.

fzaninotto avatar May 14 '24 07:05 fzaninotto

I've looked up by forwardRef usage, and most of the components work fine. Another affected component I found is ResettableTextField - it also requires props onPointerEnterCapture, onPointerLeaveCapture. So currently there are 3 components:

  1. MenuItemLink - requires onPointerEnterCapture, onPointerLeaveCapture
  2. ResettableTextField - requires onPointerEnterCapture, onPointerLeaveCapture
  3. InspectorButton - requires onPointerEnterCapture, onPointerLeaveCapture and placeholder

As I see, placeholder field from MenuItemLink is already omitted...

ilia-os avatar May 16 '24 19:05 ilia-os

Also, what would you want to "remove"?

https://github.com/marmelab/react-admin/pull/9853 works @fzaninotto

AntonOfTheWoods avatar May 17 '24 09:05 AntonOfTheWoods