nandorojo / solito

🧍‍♂️ React Native + Next.js, unified.
https://solito.dev
MIT License
3.54k stars 180 forks source link

Type Warning on SolitoImage #437

Closed timothymiller closed 1 year ago

timothymiller commented 1 year ago

Is there an existing issue for this?

Do you want this issue prioritized?

Current Behavior

When using SolitoImage with a string src, it throws the following type warnings:

Implementation:

<SolitoImage src="/logo.png" width={96} height={96} alt="App Logo" />

The warnings:

Type '{ src: string; width: number; height: number; alt: string; }' is not assignable to type 'IntrinsicAttributes & (SolitoImageProps & RefAttributes<Image>)'.

Type '{ src: string; width: number; height: number; alt: string; }' is missing the following properties from type 'Pick<any, "alt" | "blurDataURL" | "placeholder" | "loader" | "priority" | "loading" | "sizes" | "quality" | "crossOrigin" | "referrerPolicy" | "unoptimized">': blurDataURL, placeholder, loader, priority, and 6 more.ts(2322)

Workaround

The implementation works corectly, and adding @ts-ignore will temporarily make the warning go away.

Expected Behavior

The types on SolitoImage to not throw a warning when using an implementation from the docs.

Steps To Reproduce

No response

Versions

- Solito: 4.0.1
- Next.js: 13.5.3
- Expo: 49.0.13
- React Native: 0.72.5

Screenshots

Screenshot 2023-10-15 at 16 38 14

Reproduction

No response

derek-primumco commented 1 year ago

You should use @ts-expect-error The reason the error is a false alarm instead of @ts-ignore.

https://dev.to/maafaishal/ts-expect-error-over-ts-ignore-in-typescript-5140#:~:text=When%20you%20use%20%40ts%2Dignore,to%20ignore%20the%20type%20error.

ESLint rule: https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/ban-ts-comment.md#allow-with-description

nandorojo commented 1 year ago

I wonder if you have @types/react-native installed or something that's messing this up

nandorojo commented 1 year ago

I'm not able to reproduce this error. It works for me on this playground.

derek-primumco commented 1 year ago

@nandorojo I see it on that exact playground, although there was a rendering glitch for about 20 seconds while loading...

image
nandorojo commented 1 year ago

image

Works fine for me oddly

nandorojo commented 1 year ago

The glitch is due to it downloading react

derek-primumco commented 1 year ago

Well this line is the issue src: Exclude<NextImageProps['src'], string> | number 🤷

nandorojo commented 1 year ago

I wonder if it’s from a new version of next changing types?

nandorojo commented 1 year ago

Oddly I haven’t reproduced it yet

derek-primumco commented 1 year ago

Possibly. I haven't tried to debug it.

But I can confirm that we've seen this live in our codebase for months.

with @ts-expect-error

image

without @ts-expect-error

image

This is with "solito": "^4.0.1", confirmed 4.0.1 in yarn.lock.

derek-primumco commented 1 year ago

Also my previous comment wasn't the least bit off-topic -- to use Solito in a production TypeScript environment, you absolutely need to be using @ts-expect-error. If you think it's off-topic, then I'll stop contributing now. Thanks

--

You should use @ts-expect-error The reason the error is a false alarm instead of @ts-ignore.

https://dev.to/maafaishal/ts-expect-error-over-ts-ignore-in-typescript-5140#:~:text=When%20you%20use%20%40ts%2Dignore,to%20ignore%20the%20type%20error.

ESLint rule: https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/ban-ts-comment.md#allow-with-description

nandorojo commented 1 year ago

Hiding it wasn’t meant as a slight, I’m simply trying to guide the conversation of issues towards identifying the problem.

I marked it as off-topic because it was just stating a different TypeScript comment to silence the error and came off as a proposed solution.

For what it’s worth, I also prefer ts-expect-error over ts-ignore to encourage correctness when fixes are released. That said, I’ve heard it’s supposed to compile a bit slower than ts-ignore and didn’t directly get to the heart of the issue.

If you think it's off-topic, then I'll stop contributing now.

I think we should start over a bit here because it wasn’t my intention to make you feel that way. I have dozens GitHub notifications per hour that I try to respond to promptly, and part of that is just triaging comments to make sure they are key to solving the issue.

But I can confirm that we've seen this live in our codebase for months.

If it’s been in your codebase for months, I’d like to get it solved. I was under the impression that this was fixed for a while (since it’s worked for us and others on Discord).

nandorojo commented 1 year ago

In any case, I unhid @derek-primumco’s comment, but above all it would be great to find out where the root of this issue is and I’d be happy to merge a PR.

nandorojo commented 1 year ago
image

Hey guys, I'm still struggling to repro this...here it is in app code.

nandorojo commented 1 year ago

Here is an example of it working for me: https://github.com/nandorojo/solito-437-repro

I tried upgrading Next to latest and that worked too.

Can you guys try removing @types/react-native from any package.json you might have and make sure you're on RN 0.72+?

If you can create a reproduction showing this working I can take a look. However, I am simply not able to reproduce it for some reason.

Can you also make sure you're on a recent version of Expo?

npx create-solito-app repro
cd repro
cd apps/expo
expo install expo-image
cd ../.. 
yarn
code .
# open packages/features/home/screen

I then added the image component to the home screen, and it worked:

Screenshot 2023-10-17 at 10 06 05 AM
nandorojo commented 1 year ago

Possibly. I haven't tried to debug it.

But I can confirm that we've seen this live in our codebase for months.

with @ts-expect-error image

without @ts-expect-error image

This is with "solito": "^4.0.1", confirmed 4.0.1 in yarn.lock.

@derek-primumco can you post the full code? Hard to debug from just that. Looks like it's receiving src: any, not src: string, so I wonder what's going on there. The OP's use-case is using src: string.

nandorojo commented 1 year ago

@timothymiller your code is showing this error:

Type '{ src: string; width: number; height: number; alt: string; }' is missing the following properties from type 'Pick<any, "alt" | "blurDataURL" | "placeholder" | "loader" | "priority" | "loading" | "sizes" | "quality" | "crossOrigin" | "referrerPolicy" | "unoptimized">': blurDataURL, placeholder, loader, priority, and 6 more.ts(2322)

Something is clearly wrong here, because it's saying from Pick<any which would indicate some type isn't getting loaded for you. What happens if you run yarn why next and yarn why expo-image? Did you run yarn install in the root of your repository?

nandorojo commented 1 year ago

These are the props in question where the warning is thrown. It seems like Next.js is not getting properly installed in your package. Please confirm the results of yarn why next – I have a feeling this may not be a Solito starter? Or maybe you need to run yarn install in the root.

Screenshot 2023-10-17 at 10 10 25 AM

As for @derek-primumco's case, which is odd to me, it may be due to wrapping SolitoImage in such a way where the props it receives will be ambiguous, as opposed to simply spreading the props. If I see the code it's probably an easy fix.

nandorojo commented 1 year ago

I'm going to close this as I am not able to reproduce it. I'm happy to reopen it if there is a minimal repository that clones the behavior. However, it seems like it's all working as intended.

apetta commented 12 months ago

For me, ensuring expo-image was installed in the expo project resolved this error.

For what its worth, you do get a glimpse of the error on the playground @nandorojo shared, but it's just while it's figuring out the dependencies: Screenshot 2023-11-29 at 09 04 40

I think ensuring all dependencies are installed on both next/expo sides of your project should resolve this error - especially if you're not using a Solito starter.

joshuaobrien commented 10 months ago

I was experiencing this error, but ensuring expo-image was installed in the expo project resolved it for me too 👍. Looks like I missed that when setting up my project.

HaythemLazaar commented 10 months ago

I had the same error, I had to install expo-image in the Next.js project as mentioned above.

aatosm commented 10 months ago

I had the same problem using with-tailwind -starter repo. Installing expo-image resolved the problem.

nandorojo commented 9 months ago

In case anyone faces an issue even after installing expo-image, you might try this with each file type you use. Add image.d.ts in the root of your repo:

// image.d.ts
declare module '*.png' {
  const value: any
  export default value
}