eBay / nice-modal-react

A modal state manager for React.
https://ebay.github.io/nice-modal-react
MIT License
1.96k stars 109 forks source link

Type regression in 1.2.9 #127

Closed francislavoie closed 9 months ago

francislavoie commented 9 months ago

See https://github.com/eBay/nice-modal-react/pull/102#issuecomment-1477486976 for context.

Bubbling this up for visibility because the merged PR hasn't seen any activity. I still cannot upgrade nice-modal-react because of this breakage in types.

supnate commented 9 months ago

Hi @francislavoie , could you please give an example to reproduce?

supnate commented 9 months ago

Should be resolved in v1.2.11.

francislavoie commented 9 months ago

Unfortunately I still get a similar error:

TS2769: No overload matches this call.
  Overload 1 of 3, '(modal: FC<Props & NiceModalHocProps>, args?: Partial<Omit<Props & NiceModalHocProps, "id">> | undefined): Promise<...>', gave the following error.
    Argument of type '{ title: string; message: any; }' is not assignable to parameter of type 'Partial<Omit<Props & NiceModalHocProps, "id">>'.
      Object literal may only specify known properties, and 'title' does not exist in type 'Partial<Omit<Props & NiceModalHocProps, "id">>'.
  Overload 2 of 3, '(modal: string, args?: Record<string, unknown> | undefined): Promise<unknown>', gave the following error.
    Argument of type 'FC<Props & NiceModalHocProps>' is not assignable to parameter of type 'string'.
  Overload 3 of 3, '(modal: string, args: { title: string; message: any; }): Promise<unknown>', gave the following error.
    Argument of type 'FC<Props & NiceModalHocProps>' is not assignable to parameter of type 'string'.

For comparison, I would get this with v1.2.9:

TS2769: No overload matches this call.
   Overload 1 of 3, '(modal: FC<Props & NiceModalHocProps>, args?: Omit<Props & NiceModalHocProps, "id"> | undefined): Promise<...>', gave the following error. 
    Argument of type '{ title: string; content: () => JSX.Element; cancelLabel: string; }' is not assignable to parameter of type 'Omit<Props & NiceModalHocProps, "id">'.
       Object literal may only specify known properties, and 'cancelLabel' does not exist in type 'Omit<Props & NiceModalHocProps, "id">'.
   Overload 2 of 3, '(modal: string, args?: Record<string, unknown> | undefined): Promise<unknown>', gave the following error.
     Argument of type 'FC<Props & NiceModalHocProps>' is not assignable to parameter of type 'string'.
   Overload 3 of 3, '(modal: string, args: { title: string; content: () => Element; cancelLabel: string; }): Promise<unknown>', gave the following error.
     Argument of type 'FC<Props & NiceModalHocProps>' is not assignable to parameter of type 'string'.

We typically call this:

import NiceModal from "@ebay/nice-modal-react";

...

try {
  // do stuff
} catch (e) {
  NiceModal.show(ErrorModal, {
    message: e.message,
  }).catch(console.error);
}

Where ErrorModal is a simple component like:

import React, { PropsWithRef } from "react";
import NiceModal, { useModal } from "@ebay/nice-modal-react";

type Props = {
  message: string;
  invoke?: () => void;
};

export const ErrorModal = NiceModal.create(({
  message,
  invoke,
}: PropsWithRef<Props>) => {
    const modal = useModal();

    const cancel = () => {
      if (invoke) invoke();
      modal.remove();
    };

    return (
      <div>
        {message}
      </div>
    );
  }
)
supnate commented 9 months ago

I can't reproduce with your sample code, could you create a codesandbox sample?

francislavoie commented 9 months ago

Oh okay - some of the errors I was seeing were now legitimate type errors that were not properly reported in earlier versions, but others I think are bugs.

I have one like this:

        const result = await NiceModal.show<EditMessageResponse>(EditMessage, {
          title: "Edit Message",
          otherProps: "foo",
        });

It gives me this error:

TS2345: Argument of type 'FC<Props & NiceModalHocProps>' is not assignable to parameter of type 'string'.

I think there's missing an overload that allows a component and a generic promise response. The only overloads with a generic/promise takes a string as the first arg.

supnate commented 9 months ago

Can you give a full example?

francislavoie commented 9 months ago

https://codesandbox.io/p/sandbox/floral-sun-qmrsgq?file=%2Fsrc%2FApp.tsx%3A24%2C16

Running npx tsc in the terminal shows the error.

rimorin commented 9 months ago

Are there any updates on this issue? I am facing something similar issue with the modal.create typing in 1.2.11

supnate commented 9 months ago

@francislavoie for now NiceModa.show infers type of modal props and then constrains args type. So it now doesn't support a single generic type show<T>() because of limitation from: https://github.com/microsoft/TypeScript/issues/26242 .

Suggest using const result: EditMessageResponse = await NiceModal.show(EditMessage, {... instead.

supnate commented 9 months ago

@rimorin any detail or example about create typing issue?

supnate commented 9 months ago

btw, did some further typing improvement in v1.2.12.

francislavoie commented 9 months ago

Suggest using const result: EditMessageResponse = await NiceModal.show(EditMessage, {... instead.

Thanks, that makes sense :+1:

The problems are resolved for me now!

rimorin commented 9 months ago

Thanks @supnate

I have followed your suggestion and it is working now.