-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Chore(web-react): Web package is direct dependency #1288
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for spirit-design-system-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for spirit-design-system-demo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
packages/web-react/package.json
Outdated
@@ -18,6 +18,8 @@ | |||
"types": "./index.d.ts", | |||
"dependencies": { | |||
"@floating-ui/react": "^0.26.5", | |||
"@lmc-eu/spirit-web": "^1.10.0", | |||
"@lmc-eu/spirit-icons": "^1.1.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean Jobs will now install our icons too? Even when they don't use them because they use their own icons package (the same as they use their own design tokens package).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are right. I have been thinking about the Icons and move them to optionalDependencies
. But spirit-web
seems to me that is the direct dependency that is needed always.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the same manner, I am also thinking about design-tokens and icons in a web package. They both can be used or replaced by the package with the product design system definitions. So should we make them peer dependency optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@crishpeen Good point 👍🏻
Just for the record, I was thinking the optionalDependencies
property would describe it better:
"optionalDependencies": {
"@lmc-eu/spirit-web": "^1.10.0",
"@lmc-eu/spirit-icons": "^1.1.1",
}
- https://docs.npmjs.com/cli/v10/configuring-npm/package-json#optionaldependencies
- https://classic.yarnpkg.com/lang/en/docs/dependency-types/
But, as noted in the npm docs, we should check if any design tokens and icons package are installed and throw an error if they are not. No, I don't know how to do that 🙂. And, more importantly, it does not do what we want it to do: unreachable optional dependencies just do not cause the installation to fail, that's all.
So, having come this far, I think optional peerDependencies
is fine. But a check for tokens and icons from our side (instead of a broken build) would still be a nice DX improvement.
* we cannot use web-react without web and icons package * it also tells lerna how to manage builds and in what order
2cf1b61
to
154ca20
Compare
✅ Deploy Preview for spirit-design-system ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for spirit-design-system-storybook ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for spirit-design-system-validations canceled.
|
Description
Additional context
Issue reference
Before submitting the PR, please make sure you do the following
fixes #123
).