atomicojs / atomico

Atomico a micro-library for creating webcomponents using only functions, hooks and virtual-dom.
https://atomicojs.dev
MIT License
1.15k stars 43 forks source link

Types of hooks broken #90

Closed efoken closed 1 year ago

efoken commented 2 years ago

Describe the bug Since Atomico 1.62.0 many types of hooks are broken. Some errors:

useCallback:

const isYearDisabled = useCallback((dateToValidate: Date) => {
    // ...
  },
  [minDate, maxDate],
);

Argument of type '(dateToValidate: Date) => boolean' is not assignable to parameter of type '() => any'.ts(2345)

useProp does not accept callbacks anymore:

const [expanded, setExpanded] = useProp<boolean>('expanded');

// ...

setExpanded((prevExpanded) => !prevExpanded);

Argument of type '(prevExpanded: any) => boolean' is not assignable to parameter of type 'boolean'.ts(2345)

To Reproduce Steps to reproduce the behavior:

  1. Install TypeScript and Atomic 1.62.0
  2. Try one of the code snippets above in VSCode
  3. See error

Expected behavior No TypeScript errors.

Additional context I checked you types for the hooks and what you have changed.

// hooks.d.ts
export type UseCallback = <CallbackMemo extends () => any>(
    callback: CallbackMemo,
    args?: any[]
) => CallbackMemo;

// core.d.ts
export const useCallback: Hooks.UseCallback;

With the assignment the CallbackMemo gets lost, though useCallback is of type useCallback<() => any>(callback: () => any, args?: any[] | undefined): () => any

UpperCod commented 2 years ago

Thanks for your issue, I'll work on it . As a curious fact, I refactored the types to add to the Atomico pipeline a validation of use cases through Typescript.

I will inform you when the new version is available with this fix

UpperCod commented 2 years ago

Ready, fix available in version 1.62.2

efoken commented 2 years ago

Thank you for the quick fix, but we are still having some type issues with our components 😢

useProp:

const [date, setDate] = useProp<Date>('date');

const newDate = new Date();
setDate(newDate);

Argument of type 'Date' is not assignable to parameter of type '((state: undefined) => undefined) & ((state: null) => null) & (Date | ((state: Date) => Date))'.

const [expanded, setExpanded] = useProp<boolean>('expanded');

setExpanded((prevExpanded) => !prevExpanded);

Parameter 'prevExpanded' implicitly has an 'any' type.

useState:

We have a PinInput component for entering 4-digit or 6-digit codes and we use useState for setting the current array of values and we use a type with fixed array length:

const [values, setValues] = useState<
  [number, number, number, number] | [number, number, number, number, number, number]
  >([1, 2, 3, 4]);

// ...

setValues([0, 0, 0, 0]);

Argument of type '[number, number, number, number]' is not assignable to parameter of type '([number, number, number, number] & ((state: [number, number, number, number, number, number]) => [number, number, number, number, number, number])) | (((state: [number, number, number, number]) => [...]) & [...]) | (((state: [...]) => [...]) & ((state: [...]) => [...]))'.

This worked before 1.62.0

I think all other issues we have are related to those two, especially the first one.

UpperCod commented 2 years ago

Thanks for notifying it, I have already fixed it in 1.62.3, I have also added the cases you share to the typescript validation pipeline before publication

I will keep an eye on your Atomico update status 🤓

efoken commented 1 year ago

Okay seems to work now, with one little change that I did. But I'm not sure about that one... maybe you can help me.

Before:

const [value, setValue] = useProp<string>('value');

const handleChange = (event: Event) => {
  setValue((event.currentTarget as HTMLElement).getAttribute('value'));
}

The getAttribute('value') has a return type of string | null. So I got an error which makes sense:

Type 'null' is not assignable to type 'string | ((reduce: string) => string)'.

So I changed this to:

const [value, setValue] = useProp<string>('value');

const handleChange = (event: Event) => {
  setValue((event.currentTarget as HTMLElement).getAttribute('value') ?? '');
}

But what's the correct way here? Should I set the prop to null or an empty string?

UpperCod commented 1 year ago

Thanks for reporting, I've fixed it in version 1.62.4, how about we improve the development experience with the following change:

Now

const [  value, setValue ] = useProp<number>("prop");

setValue(null) // TS ✅

setValue(undefined) // TS ✅

setValue((value = 0 )=>value+1) // TS ⚠️, value can be null

Proposal

setValue((value = 0 )=>value+1) // TS ✅

This allows all setStates as reducers to have default parameters.

efoken commented 1 year ago

Nice thanks 🙏

Yeah I guess that would be a nice change, I never thought about passing default parameters like that, nice 👍

UpperCod commented 1 year ago

I just received this news https://ibmix.de/en/blog/red-dot-design-award-for-barmer/, Congratulations🎉

@efoken would be pleased to have your presence on the Discord channel to learn more about your user experience.

Personally, I would very much like to be able to share your case more extensively on the Atomico site, I can create a space on github discussions to document your case, are you okay with that?

efoken commented 1 year ago

Thank you 🎉

I am currently not active on Discord, but sure I can join your channel.

And of course you can create a discussion space for us, I can also invite the other developers to contribute in documenting our case.

UpperCod commented 1 year ago

Ready👍, I have already created the space to learn more about its use case, https://github.com/atomicojs/atomico/discussions/92 I will be happy that you are there and can add the other developers. for my part I will be adding questions over the course of the week