Decathlon / vitamin-web

Decathlon Design System UI components for web applications
https://decathlon.github.io/vitamin-web
Apache License 2.0
279 stars 77 forks source link

bug: Type error with VitamixId in vtmn/react v.1.1.0 #1407

Closed JulienGaudet closed 1 year ago

JulienGaudet commented 1 year ago

Describe the bug Hello there ! While bumping vtmn/react from 0.54.3 to 1.1.0 I get a typing error with the VitamixId property :

Type 'import("/Users/JULIEN/Documents/GitHub/save-the-stock-front/node_modules/.pnpm/@vtmn+icons@1.1.0/node_modules/@vtmn/icons/dist/vitamix/font/vitamix").VitamixId' is not assignable to type 'import("/Users/JULIEN/Documents/GitHub/save-the-stock-front/node_modules/.pnpm/@vtmn+icons@0.21.8/node_modules/@vtmn/icons/dist/vitamix/font/vitamix").VitamixId'. Type '"accessibility-fill"' is not assignable to type 'VitamixId'.ts(2322) VtmnIcon.d.ts(24, 5): The expected type comes from property 'value' which is declared here on type 'IntrinsicAttributes & VtmnIconProps'

Steps to reproduce

  1. Use a VitamixId Prop to set the value for the VtmnIcon component

It also seems that the brand variant does not work anymore, the icons now appear in black and not with the old brand blue. Any idea on how to fix those ?

Thank you :)

thibault-mahe commented 1 year ago

Hello @JulienGaudet Thanks for your message :) I can't reproduce this problem unfortunately though. In your message there seems to be a mismatch between the versions of @vtmn/icons: @vtmn/react ^1.1.0 uses @vtmn/icons ^1.1.0, but you may still have an old version of @vtmn/icons. Did you eventually try to bump it in your package.json?

JulienGaudet commented 1 year ago

Thank you for your quick response, yes I try to bump all versions at the same time. I will try again right now

JulienGaudet commented 1 year ago

Good call, one reference of the old package wasn't updated on the package lock json. I had to remove it by hand and delete the node modules to remove all references.

Jerome1337 commented 1 year ago

Hi @thibault-mahe, it seems to have a version mismatch since 2.0.0 has been released.

@vtmn/icons doesn't have a 2.0.0 version then when we use @vtmn/react ^2.0.0 we get the same error as mentioned above. But in fact @vtmn/react ^1.1.0 + @vtmn/icons ^1.1.0 works.

Is there a release tag missing on @vtmn/icons?

lauthieb commented 1 year ago

Hi @Jerome1337,

@vtmn/icons doesn't have version 2.0.0, that's normal, package versioning is independent (and there wasn't any breaking change for icons).

Can you please send us your error with log trace?

For your information, the bump to react@2.0.0 only did this: https://github.com/Decathlon/vitamin-web/commit/25a7c16abf352170e4936aacb6b674aec54e3067 (Skeleton component, nothing related to icons). I also made this PR to align our internal dependencies to the latest available: https://github.com/Decathlon/vitamin-web/pull/1415.

Jerome1337 commented 1 year ago

Hi @lauthieb,

Here is the stack trace when trying to pass a custom props that has the @vtmn/icons VitamixId type to iconLeft iconRight iconAlone of the VtmnButton

Type 'import("/Users/JEROME/Documents/Projects/oneSquare/front/node_modules/@vtmn/icons/dist/vitamix/font/vitamix").VitamixId | undefined' is not assignable to type 'import("/Users/JEROME/Documents/Projects/oneSquare/front/node_modules/@vtmn/react/node_modules/@vtmn/icons/dist/vitamix/font/vitamix").VitamixId | undefined'.
  Type '"accessibility-fill"' is not assignable to type 'VitamixId | undefined'.

But when downgrading @vtmn/react to v1.1.0 every is working

lauthieb commented 1 year ago

Thanks @Jerome1337 do you have two different versions of @vtmn/icons in your lock file (package-lock.json or yarn.lock)?

Jerome1337 commented 1 year ago

Yes because @vtmn/react ^2.0.0 seems to require @vtmn/icons ^0.21.8

"@vtmn/icons@npm:^0.21.8":
  version: 0.21.8
  resolution: "@vtmn/icons@npm:0.21.8"
  checksum: eb6b8a05630214af831d65d660e319d89ab9cfad457e96f0feea54d4b7a86cf098327e2faf9d1bfcc03c5877fec3ddb59c6820f283e026e0781d229e79c8f152
  languageName: node
  linkType: hard

"@vtmn/icons@npm:^1.1.0":
  version: 1.1.0
  resolution: "@vtmn/icons@npm:1.1.0"
  checksum: fc7adfe1d9f53fedc6bfb06e2f0494d72405639b46eacc218dc38fed444e51ca2342054e51378d545e641dd6545ff9635f81c0fc04fc8ba2d6f7568ca55f92fe
  languageName: node
  linkType: hard

"@vtmn/react@npm:^2.0.0":
  version: 2.0.0
  resolution: "@vtmn/react@npm:2.0.0"
  dependencies:
    "@vtmn/icons": ^0.21.8
  peerDependencies:
    react: ">=16.8.0"
    react-dom: ">=16.8.0"
  checksum: bba5ebbe83f2acc25a7ff48509d72adc1bd86c07d1df4e79f21ea07feb6a3a453f7677bde0ec90da5c732308a7ff2c588a668f7950f98105f02e8310a54d9e3d
  languageName: node
  linkType: hard
lauthieb commented 1 year ago

I think I found the problem, that's because the "accessibility-fill" icon is a new icon and was not available in your version @vtmn/icons@npm:^0.21.8. This PR should fix the problem: https://github.com/Decathlon/vitamin-web/pull/1415 Sorry I have two meetings, can you please review this PR? I will come back to you after.

JulienGaudet commented 1 year ago

Yep, I have the same problem as @Jerome1337. Every time I do a npm i, I have to remove the dependencies: "@vtmn/icons": ^0.21.8 in the lock and delete the node modules.

Seems like there's still a reference of that old version somewhere. 
lauthieb commented 1 year ago

@JulienGaudet, yes and that's why this PR https://github.com/Decathlon/vitamin-web/pull/1415 should solve your problem. Can you also please review it? We will merge it this morning with @thibault-mahe Btw, thank you @Jerome1337 for your approval.

JulienGaudet commented 1 year ago

It's done, thank you :) !