iTwin / viewer

Monorepo that contains the iTwin Viewer npm packages and their related packages
MIT License
24 stars 15 forks source link

Add missing `redux` dependencies and `imodels-client-management` peer dependency #221

Closed GytisCepk closed 1 year ago

GytisCepk commented 1 year ago

Changes:

aruniverse commented 1 year ago

ive been going back and forth so much with this pr; i hate peer deps and statics sooo so much.

redux/react-redux changes look fine

re the imodels-* pkgs:

heres a partial patch of what i think the changes should probably look like viewer-imodels-pr.txt

lmk what your thoughts are

aruniverse commented 1 year ago

heres a partial patch of what i think the changes should probably look like viewer-imodels-pr.txt

looks like in my patch, i removed the direct dep in web-viewer & desktop-viewer react and just kept the direct deps at the test app/template levels. so the opposite of what i said yesterday... idk what was going on when i was wiritng that out yesterday.... sorry

in the patch, the imodels-client-management copies what we do for imodels-access-frontend.

as mentioned in todays call someone does need to have a direct dep on the client pkg, either web/desktop-viewer or the test app. im questioning if the imodels-access-frontend should be a direct dep in the web/desktop as well if we make the client a direct dep. im going to approve this pr for now, but i think we need to take a better look at these deps when we update to the 2.x imdoels pkgs