mui / toolpad

Toolpad: Full stack components and low-code builder for dashboards and internal apps.
https://mui.com/toolpad/
MIT License
1.27k stars 282 forks source link

Ctrl + Click on link doesn't work #1217

Closed oliviertassinari closed 2 years ago

oliviertassinari commented 2 years ago

Duplicates

Latest version

Steps to reproduce 🕹

Steps:

  1. Open https://master--toolpad.mui.com/deploy/cl6rqzry10009arlv9sto6qja/pages
  2. Ctrl + Click on one of the pages

Current behavior 😯

It opens in the same tab

Expected behavior 🤔

It opens in a new tab

Context 🔦

I was trying to open all these apps in a different tab. I also can't use the context menu as it's not a link:

Screenshot 2022-10-26 at 17 09 56

The root of the issue is here:

https://github.com/mui/mui-toolpad/blob/77f393df3e363e0fec147a1d522dcd2f2623b8ad/packages/toolpad-app/src/runtime/AppOverview.tsx#L12-L15

I think that we should use https://reactrouter.com/en/main/components/link. As a side note, I don't think that useCallback help here, it's a simple function, it's simpler without and maybe faster to create at each render than to memo.

Your environment 🌎

No response

Janpot commented 2 years ago

@oliviertassinari

As a side note, I don't think that useCallback help here, it's a simple function, it's simpler without and maybe faster to create at each render than to memo.

True, I tend to memoize all (most) functions and objects because I don't know how the downstream components deal with them. as an example from the DataGrid docs:

Always memoize the function provided to getRowHeight. The grid bases on the referential value of these props to cache their values and optimize the rendering.

Memoizing everything prevents me from having to look up documentation or in worst case the implementation of all components. It prevents component libraries or maintainers from making semver patch level changes that accidentally kill performance.

I haven't benchmarked the function creation overhead, but I've never seen any useMemo call come up significantly in any flamegraph.

This is a controversial topic and I realize this is a strong opinion. I'm happy to adapt though.

apedroferreira commented 2 years ago

This is a controversial topic and I realize this is a strong opinion. I'm happy to adapt though.

I've never ran into any situation where useCallback or useMemo is causing any significant difference in performance compared to just a function, so I also tend to always use useCallback in functions inside components. It also keeps me from having to think if it's worth using useCallback in every single case. So I'm fine with the pattern of always using them.

oliviertassinari commented 2 years ago

I haven't benchmarked the function creation overhead, but I've never seen any useMemo call come up significantly in any flamegraph.

@Janpot Agree, and true the other way around, in most cases, not using useMemo/useCallback has no impacts.

I think that it's a tension between these 3 things, for the rest it doesn't matter:

  1. You need useCallback when your component would be buggy without. For MUI, we are trying for this to almost never happen. For example, https://github.com/mui/mui-x/pull/6185 made progress in working better without useCallback/memo. We keep track of this at https://github.com/mui/mui-x/issues/147.
  2. You need useCallback when you have a component that is high in the React tree and would lead to a lot of work if you don't.
  3. But each time we opt for useCallback it's more boilerplate to read and write.