[Link] Improve integration with Next.js

kelvinsanchez15 commented 3 years ago

[x] The issue is present in the latest release.
[x] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Passing an URL object to the MUI button component, as in the code below, works without problems but you get the following error: Warning: Failed prop type: Invalid prop `href` of type `object` supplied to `ForwardRef(ButtonBase)`, expected `string`.

// index.js
import React from "react";
import Button from "@material-ui/core/Button";
import Link from "../src/Link";

export default function Index() {
  return (
    <div>
      <Button
        component={Link}
        href={{
          pathname: "/about",
          query: { name: "test" }
        }}
        naked
        variant="contained"
      >
        Link button with url object
      </Button>
    </div>
  );
}

Note: The imported Link component is the same as the one in the NextJS - Material UI integration example.

Expected Behavior 🤔

No error should appear.

Steps to Reproduce 🕹

CodeSandbox reproduction: https://codesandbox.io/s/next-materialui-objecturl-rmprr?file=/pages/index.js

Context 🔦

In the NextJS documentation I learned that we can use an URL object and it will get formatted automatically to create an URL string.

Using URL objects instead of strings has helped me to handle complex queries mutations easily and intuitively.

Your Environment 🌎

`npx @material-ui/envinfo`

``` System: OS: Windows 10 10.0.17763 Binaries: Node: 15.0.1 - C:\Program Files\nodejs\node.EXE Yarn: Not Found npm: 6.14.8 - C:\Program Files\nodejs\npm.CMD Browsers: Chrome: Not Found Edge: Not Found npmPackages: @material-ui/core: ^4.11.2 => 4.11.2 @material-ui/icons: ^4.11.2 => 4.11.2 @material-ui/lab: ^4.0.0-alpha.57 => 4.0.0-alpha.57 @material-ui/styles: 4.11.2 @material-ui/system: 4.11.2 @material-ui/types: 5.1.0 @material-ui/utils: 4.11.2 @types/react: 17.0.0 react: ^17.0.1 => 17.0.1 react-dom: ^17.0.1 => 17.0.1 ```

oliviertassinari commented 3 years ago

@kelvinsanchez15 Interesting, thanks for raising. I can only think of two solutions here. Either we remove/soften the prop-type for href or you use a different prop, like to. Maybe the former?

kelvinsanchez15 commented 3 years ago

I just tried removing the prop-type and also making both strings and objects valid. Either works pretty well, I can submit a PR if you think it's okay or we can wait for more feedback. @oliviertassinari

These were the files that I modified:

// node_modules\@material-ui\core\ButtonBase\ButtonBase.d.ts

export type ExtendButtonBase<M extends OverridableTypeMap> = ((
  props: { href: string | object} & OverrideProps<ExtendButtonBaseTypeMap<M>, 'a'>
) => JSX.Element) &
  OverridableComponent<ExtendButtonBaseTypeMap<M>>;

// node_modules\@material-ui\core\ButtonBase\ButtonBase.js

href: _propTypes.default.oneOfType([_propTypes.default.string, _propTypes.default.object]),

// node_modules\@material-ui\core\esm\ButtonBase\ButtonBase.js

href: PropTypes.oneOfType([PropTypes.string, PropTypes.object]),

oliviertassinari commented 3 years ago

@kelvinsanchez15 I think that we can simply remove the prop type. It doesn't seem that we need it. We could also add a test case to avoid regressions if we move in this direction. Did you check that it work with TypeScript?

kelvinsanchez15 commented 3 years ago

@kelvinsanchez15 I think that we can simply remove the prop type. It doesn't seem that we need it. We could also add a test case to avoid regressions if we move in this direction.

Yeah, that will work.

Did you check that it work with TypeScript?

I'm sorry, I have no experience working with typescript yet, just general knowledge.

oliviertassinari commented 3 years ago

@kelvinsanchez15 Ok, I had a look at the types. It seems that we can improve this version: https://github.com/mui-org/material-ui/blob/70e6c6b7268918502f9c82b2820146d3a1f2ff33/examples/nextjs-with-typescript/src/Link.tsx with:

/* eslint-disable jsx-a11y/anchor-has-content */
import * as React from 'react';
import clsx from 'clsx';
import { useRouter } from 'next/router';
import NextLink, { LinkProps as NextLinkProps } from 'next/link';
import MuiLink, { LinkProps as MuiLinkProps } from '@material-ui/core/Link';

type NextComposedProps = Omit<React.AnchorHTMLAttributes<HTMLAnchorElement>, 'href'> & NextLinkProps;

const NextComposed = React.forwardRef<HTMLAnchorElement, NextComposedProps>((props, ref) => {
  const { as, href, replace, scroll, passHref, shallow, prefetch, ...other } = props;

  return (
    <NextLink
      href={href}
      prefetch={prefetch}
      as={as}
      replace={replace}
      scroll={scroll}
      shallow={shallow}
      passHref={passHref}
    >
      <a ref={ref} {...other} />
    </NextLink>
  );
});

export type LinkProps = {
  activeClassName?: string;
  href: NextLinkProps['href'];
  naked?: boolean;
} & Omit<NextComposedProps, 'to'> & MuiLinkProps;

// A styled version of the Next.js Link component:
// https://nextjs.org/docs/#with-link
const Link = React.forwardRef<HTMLAnchorElement, LinkProps>(function Link(props, ref) {
  const { href, activeClassName = 'active', className: classNameProps, naked, ...other } = props;

  const router = useRouter();
  const pathname = typeof href === 'string' ? href : href.pathname;
  const className = clsx(classNameProps, {
    [activeClassName]: router.pathname === pathname && activeClassName,
  });

  if (naked) {
    return <NextComposed className={className} ref={ref as any} href={href} {...other} />;
  }

  return <MuiLink component={NextComposed} className={className} ref={ref} href={href} {...other} />;
});

export default Link;

It solves all the type issues. The object syntax was working and is still work. It seems that we can do:

diff --git a/packages/material-ui/src/ButtonBase/ButtonBase.js b/packages/material-ui/src/ButtonBase/ButtonBase.js
index 5b2517d6b2..e3ad6ed9eb 100644
--- a/packages/material-ui/src/ButtonBase/ButtonBase.js
+++ b/packages/material-ui/src/ButtonBase/ButtonBase.js
@@ -419,7 +419,7 @@ ButtonBase.propTypes = {
   /**
    * @ignore
    */
-  href: PropTypes.string,
+  href: PropTypes /* @typescript-to-proptypes-ignore */.any,
   /**
    * @ignore
    */

Do you want to work on the issue? :)

kelvinsanchez15 commented 3 years ago

Yeah I would like it. Just to be clear, I would have to update both NextJS examples (with and without TypeScript), and replace the line of code in the ButtonBase.js file?

oliviertassinari commented 3 years ago

@kelvinsanchez15 Correct, this would make the Button behaves like the Link component.

kelvinsanchez15 commented 3 years ago

While testing the changes I realized that the problem extends to the Button.js module as well. It is fixed by making the same change as in ButtonBase.js.

The other thing that can cause problems in the future is that TypeScript keeps waiting for a 'string' and although it does not appear in the console, it gets linted by VSCode:

(property) href: string
The URL to link to when the button is clicked. If defined, an a element will be used as the root node.

No overload matches this call.
  Overload 3 of 3, '(props: DefaultComponentProps<ExtendButtonBaseTypeMap<ButtonTypeMap<{}, "button">>>): Element', gave the following error.
    Type '{ pathname: string; query: { name: string; }; }' is not assignable to type 'string'.ts(2769)
Button.d.ts(42, 5): The expected type comes from property 'href' which is declared here on type 'IntrinsicAttributes & { children?: ReactNode; color?: "inherit" | "primary" | "secondary" | "default" | undefined; disabled?: boolean | undefined; ... 7 more ...; variant?: "text" | ... 2 more ... | undefined; } & { ...; } & CommonProps<...> & Pick<...>'

CodeSandbox reproduction: https://codesandbox.io/s/musing-shockley-5phqq?file=/pages/index.tsx

Probably we need to change the types on Button.d.ts and ButtonBase.d.ts files or find another way around it. What do you think? @oliviertassinari

oliviertassinari commented 3 years ago

@kelvinsanchez15 Weird, it seemed to work with the Link component.

oliviertassinari commented 3 years ago

@kelvinsanchez15 You were right, my initial proposal wasn't working. It seems that we have to change the name of the prop. What do you think about this demo? :D

https://codesandbox.io/s/blissful-bush-pgd1e?file=/pages/index.tsx

/* eslint-disable jsx-a11y/anchor-has-content */
import * as React from "react";
import clsx from "clsx";
import { useRouter } from "next/router";
import NextLink, { LinkProps as NextLinkProps } from "next/link";
import MuiLink, { LinkProps as MuiLinkProps } from "@material-ui/core/Link";
import MuiButton from "@material-ui/core/Button";

type NextLinkComposedProps = Omit<
  React.AnchorHTMLAttributes<HTMLAnchorElement>,
  "href"
> &
  Omit<NextLinkProps, "href"> & { to: NextLinkProps["href"]; href?: any };

export const NextLinkComposed = React.forwardRef<
  HTMLAnchorElement,
  NextLinkComposedProps
>((props, ref) => {
  const {
    to,
    as,
    href,
    replace,
    scroll,
    passHref,
    shallow,
    prefetch,
    ...other
  } = props;

  return (
    <NextLink
      href={to}
      prefetch={prefetch}
      as={as}
      replace={replace}
      scroll={scroll}
      shallow={shallow}
      passHref={passHref}
    >
      <a ref={ref} {...other} />
    </NextLink>
  );
});

export type LinkProps = {
  activeClassName?: string;
  naked?: boolean;
} & Omit<NextLinkComposedProps, "to"> &
  Omit<MuiLinkProps, "href">;

// A styled version of the Next.js Link component:
// https://nextjs.org/docs/#with-link
const Link = React.forwardRef<HTMLAnchorElement, LinkProps>(function Link(
  props,
  ref
) {
  const {
    href,
    activeClassName = "active",
    className: classNameProps,
    naked,
    ...other
  } = props;

  const router = useRouter();
  const pathname = typeof href === "string" ? href : href.pathname;
  const className = clsx(classNameProps, {
    [activeClassName]: router.pathname === pathname && activeClassName
  });

  if (naked) {
    return (
      <NextLinkComposed
        className={className}
        ref={ref as any}
        to={href}
        {...other}
      />
    );
  }

  return (
    <MuiLink
      component={NextLinkComposed}
      className={className}
      ref={ref}
      to={href}
      {...other}
    />
  );
});

export default function Index() {
  return (
    <div>
      <MuiButton
        component={NextLinkComposed}
        to={{
          pathname: "/about",
          query: { name: "test" }
        }}
      >
        MuiButton link
      </MuiButton>
      <br />
      <MuiLink
        component={NextLinkComposed}
        to={{
          pathname: "/about",
          query: { name: "test" }
        }}
      >
        MuiLink lin
      </MuiLink>
      <br />
      <Link
        href={{
          pathname: "/about",
          query: { name: "test" }
        }}
      >
        Wrapped Link
      </Link>
    </div>
  );
}

I still think that we can apply:

diff --git a/packages/material-ui/src/ButtonBase/ButtonBase.js b/packages/material-ui/src/ButtonBase/ButtonBase.js
index 5b2517d6b2..e3ad6ed9eb 100644
--- a/packages/material-ui/src/ButtonBase/ButtonBase.js
+++ b/packages/material-ui/src/ButtonBase/ButtonBase.js
@@ -419,7 +419,7 @@ ButtonBase.propTypes = {
   /**
    * @ignore
    */
-  href: PropTypes.string,
+  href: PropTypes /* @typescript-to-proptypes-ignore */.any,
   /**
    * @ignore
    */

for JS only users and consistency with Link.

kelvinsanchez15 commented 3 years ago

@oliviertassinari I think it is okay to rename the property as 'to', but looking at your demo, It looks like we would need to export two Link components (Link as default and NextLinkComposed). I forked your demo to play with it: https://codesandbox.io/s/admiring-wave-4oxdv?file=/pages/index.tsx

So right now I can see two options:

JS and TS users using NextLinkComposed if they need to pass an object URL to a MUI component.
If we apply the type change to Button.js and ButtonBase.js files, only TS users will need to use NextLinkComposed.

The first option is more consistent for both users, the second makes JS users able to use the 'href' prop without problems (Just like the NextJs docs explains).

oliviertassinari commented 3 years ago

@kelvinsanchez15 Yes, the new codesandbox is exactly what I had in mind for the improved version of the Next's examples.

In your initial sandbox, you did MuiButton > MuiLink > NextLinkComposed > NextLink, this is wrong. NextLinkComposed is meant for recreating new styled link components, it depends on to.

The Link component is the version that does: Link > MuiLink > NextLinkComposed > NextLink.
The same pattern can be used to create a new button link: NextButton > MuiButton > NextLinkComposed > NextLink.

I think that the correct option is n°3:

update the Next.js example with your latest codesandbox
update prop-types for consistency between MuiButtonBase and MuiLink
teach developers they can use NextLinkComposed. We can probably add a quick note in the readme of the two Next.js examples.

kelvinsanchez15 commented 3 years ago

Perfect, it seems like the right option to me too. @oliviertassinari

I just did a JS version, the only thing I'm not totally sure about, are both NextLinkComposed.propTypes and Link.propTypes objects, so tell me if changes are need it there: https://codesandbox.io/s/next-materialui-objecturl-forked-7qk0b?file=/src/Link.js

oliviertassinari commented 3 years ago

@kelvinsanchez15 For the prop-types, I would do

NextLinkComposed.propTypes = {
  as: PropTypes.oneOfType([PropTypes.string, PropTypes.object]),
- href: PropTypes.oneOfType([PropTypes.string, PropTypes.object]),
  prefetch: PropTypes.bool
};

because href is not part of the public API of this component. All good otherwise

kelvinsanchez15 commented 3 years ago

@oliviertassinari I made a PR with the changes we settle :) https://github.com/mui-org/material-ui/pull/24121

chadini12345 commented 9 months ago

Hi team , can someone help me in accessing the query param in the component sent via href. in next 14

i am writing like this

<Link // href={accounts/edit?data=${dataForEdit}} href={{ pathname: "/accounts/edit", query: { data: JSON.stringify(dataForEdit), }, }} as={/accounts/edit}

mui / material-ui