jpmorganchase / salt-ds

React UI components built with a focus on accessibility, customisation and ease-of-use
https://www.saltdesignsystem.com
Apache License 2.0
111 stars 78 forks source link

@salt-ds/core doesn't have @salt-ds/theme as dependency #2555

Open origami-z opened 9 months ago

origami-z commented 9 months ago

Package name(s)

Core (@salt-ds/core)

Package version(s)

1.9.0

Description

Component CSS depends on availability of theme tokens, so not all version is compatible.

Steps to reproduce

npm view @salt-ds/core

dependencies:
@floating-ui/react: ^0.23.0 @salt-ds/icons: ^1.7.0      @salt-ds/styles: ^0.1.1     @salt-ds/window: ^0.1.1     clsx: ^2.0.0       

Expected behavior

version X of core should depends on minimum version Y of theme

Operating system

Browser

Are you a JPMorgan Chase & Co. employee?

james-nash commented 8 months ago

While I agree we should make the compatible version ranges between theme & core explicit, I wonder if it should be the other way around: I.e. @salt-ds/theme should have a (peer?) dependency on the relevant versions of @salt-ds/core.

This might make things easier for other themes in the future. I'm assuming an alternate Salt theme would likely be distributed as its own NPM package which would be used instead of Salt's default theme one. So expressing the dependency that way around, means consumers using their own themes don't need to also install a redundant copy of Salt's default theme.

joshwooding commented 8 months ago

I don't know if using dependencies to manage this is correct. Technically that is no direct dependency between the libraries even though there theoretically is.

origami-z commented 8 months ago

Let's focus on addressing the problem right now, when components package are updated to take in new tokens, how do we communicate theme package also needs to be updated.

james-nash commented 8 months ago

At a minimum, the release notes ought to mention that and inform users which corresponding version of the theme package they need to upgrade to.

Perhaps we can also maintain a compatibility table on the site and/or internal StackOverflow, which lists which version ranges of core, go with which theme ones.

Do the other Salt packages like lab, data-grid, and ag-grid-theme have similar dependencies? If so, do we need to convey / document that somehow too?

origami-z commented 7 months ago

Another team bumps into this issue, where core package is upgraded to use new text tokens added in @salt-ds/theme@1.10.0, but theme package is not bumped, resulting a few components showing wrong result.