constgen / neutrino-storybook

GNU General Public License v3.0
1 stars 3 forks source link

Too many addons? #1

Open jayliu50 opened 3 years ago

jayliu50 commented 3 years ago

Is there a reason why you include so many add-ons by default in preset-storybook-react ?

As someone using this it just grows and complicates the dependency list. Also, my project also has dependency on the same add-on, so now there is a version mismatch that I have to resolve.

Please don't get me wrong; thank you for you work on this. It's super helpful for me!

$ yarn list --pattern "storybook"
yarn list v1.22.5
├─ @storybook/addon-actions@6.1.21
│  ├─ @storybook/channel-postmessage@6.1.21
│  ├─ @storybook/channels@6.1.21
│  ├─ @storybook/client-api@6.1.21
│  ├─ @storybook/components@6.1.21
│  └─ @storybook/theming@6.1.21
├─ @storybook/addon-backgrounds@6.0.22
│  ├─ @storybook/addons@6.0.22
│  ├─ @storybook/api@6.0.22
│  ├─ @storybook/client-logger@6.0.22
│  ├─ @storybook/core-events@6.0.22
│  └─ @storybook/router@6.0.22
├─ @storybook/addon-controls@6.0.22
│  ├─ @storybook/addons@6.0.22
│  ├─ @storybook/api@6.0.22
│  ├─ @storybook/client-logger@6.0.22
│  ├─ @storybook/core-events@6.0.22
│  ├─ @storybook/node-logger@6.0.22
│  └─ @storybook/router@6.0.22
├─ @storybook/addon-docs@6.0.22
│  ├─ @storybook/addons@6.0.22
│  ├─ @storybook/api@6.0.22
│  ├─ @storybook/client-logger@6.0.22
│  ├─ @storybook/core-events@6.0.22
│  ├─ @storybook/core@6.0.22
│  ├─ @storybook/node-logger@6.0.22
│  ├─ @storybook/router@6.0.22
│  └─ @storybook/ui@6.0.22
├─ @storybook/addon-essentials@6.1.21
│  ├─ @storybook/addon-backgrounds@6.1.21
│  ├─ @storybook/addon-controls@6.1.21
│  ├─ @storybook/addon-docs@6.1.21
│  ├─ @storybook/addon-toolbars@6.1.21
│  ├─ @storybook/addon-viewport@6.1.21
│  ├─ @storybook/channel-postmessage@6.1.21
│  ├─ @storybook/channels@6.1.21
│  ├─ @storybook/client-api@6.1.21
│  ├─ @storybook/components@6.1.21
│  ├─ @storybook/postinstall@6.1.21
│  ├─ @storybook/source-loader@6.1.21
│  └─ @storybook/theming@6.1.21
├─ @storybook/addon-links@6.1.21
├─ @storybook/addon-toolbars@6.0.22
│  ├─ @storybook/addons@6.0.22
│  ├─ @storybook/api@6.0.22
│  ├─ @storybook/client-logger@6.0.22
│  ├─ @storybook/core-events@6.0.22
│  └─ @storybook/router@6.0.22
├─ @storybook/addon-viewport@6.0.22
│  ├─ @storybook/addons@6.0.22
│  ├─ @storybook/api@6.0.22
│  ├─ @storybook/client-logger@6.0.22
│  ├─ @storybook/core-events@6.0.22
│  └─ @storybook/router@6.0.22
├─ @storybook/addons@6.1.21
│  ├─ @storybook/channels@6.1.21
│  └─ @storybook/theming@6.1.21
├─ @storybook/api@6.1.21
│  ├─ @storybook/channels@6.1.21
│  └─ @storybook/theming@6.1.21
├─ @storybook/channel-postmessage@6.0.22
│  ├─ @storybook/client-logger@6.0.22
│  └─ @storybook/core-events@6.0.22
├─ @storybook/channels@6.0.22
├─ @storybook/client-api@6.0.22
│  ├─ @storybook/addons@6.0.22
│  ├─ @storybook/api@6.0.22
│  ├─ @storybook/client-logger@6.0.22
│  ├─ @storybook/core-events@6.0.22
│  └─ @storybook/router@6.0.22
├─ @storybook/client-logger@6.1.21
├─ @storybook/components@6.0.22
│  └─ @storybook/client-logger@6.0.22
├─ @storybook/core-events@6.1.21
├─ @storybook/core@6.1.21
│  ├─ @storybook/channel-postmessage@6.1.21
│  ├─ @storybook/channels@6.1.21
│  ├─ @storybook/client-api@6.1.21
│  ├─ @storybook/components@6.1.21
│  └─ @storybook/theming@6.1.21
├─ @storybook/csf@0.0.1
├─ @storybook/node-logger@6.1.21
├─ @storybook/postinstall@6.0.22
├─ @storybook/react@6.1.21
├─ @storybook/router@6.1.21
├─ @storybook/semver@7.3.2
├─ @storybook/source-loader@6.0.22
│  ├─ @storybook/addons@6.0.22
│  ├─ @storybook/api@6.0.22
│  ├─ @storybook/client-logger@6.0.22
│  ├─ @storybook/core-events@6.0.22
│  └─ @storybook/router@6.0.22
├─ @storybook/theming@6.0.22
│  └─ @storybook/client-logger@6.0.22
├─ @storybook/ui@6.1.21
│  ├─ @storybook/channels@6.1.21
│  ├─ @storybook/components@6.1.21
│  └─ @storybook/theming@6.1.21
├─ neutrino-middleware-storybook@1.0.2
└─ neutrino-preset-storybook-react@1.0.2
   ├─ @storybook/addon-actions@6.0.22
   ├─ @storybook/addons@6.0.22
   ├─ @storybook/api@6.0.22
   ├─ @storybook/client-logger@6.0.22
   ├─ @storybook/core-events@6.0.22
   └─ @storybook/router@6.0.22
constgen commented 3 years ago

The only reason is to be more zero configurable. But lets see your package.json to understand the issue more clearly. Can you share it?

jayliu50 commented 3 years ago

Yes, thank you for attention, and your patience. I know I could be explaining more clearly.

I will attach the package.json file soon. But for now, know that in the output of yarn list above, all the 6.1.21 is from my project and the 6.0.22 are from this project. All the version numbers should be the same, but as you can see there is a mix of jumbled versions. All I need in my project are @storybook/addon-essentials and @storybook/addon-links.

And that really is the issue, version mismatch, causing a lot of weird errors. I know I could perhaps put @storybook/addons you mention in my own package.json in the resolution object, to force your project to match the version of my own project, but I feel should be unnecessary if they are not even dependencies of my own project.

Additionally, I see that in docs.js there is configuration for mdx etc. But what if I don't have need for mdx? While that config is valuable (especially because I'm still ignorant of webpack) shouldn't stuff like that be conditional on whether the project downstream actually uses it?

On Sun, Mar 14, 2021, 9:04 AM Constantine Genchevsky < @.***> wrote:

The only reason is to be more zero configurable. But lets see your package.json to understand the issue more clearly. Can you share it?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/constgen/neutrino-storybook/issues/1#issuecomment-798903396, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFEQRTJPB5LSK3GAIVYMR3TDSX5LANCNFSM4ZEIQHYQ .

constgen commented 3 years ago

Really need to see package.json.

If you don't use MDX you will even not feel it is present in the configuration. Just don't write your stories on MDX.

The included addons are already essential so you don't need to include essential addons by yourself the second time. But yes, we need to think about versioning and upgrades. Because currently they are fixated to "6.0.22" and can be upgraded only when the preset is upgraded. Probably it is better to set them "^6.0.0" in this preset. The second option to exclude them at all also can be considered

jayliu50 commented 3 years ago

The included addons are already essential so you don't need to include essential addons by yourself the second time.

But if I'm a dev using neutrino-storybook, why should I need to know this? As a dev using neutrino-storybook, I shouldn't need to know or be thinking about what neutrino-storybook has already included, but be thinking about my own project and the dependencies there. To me, it makes more sense architecturally such that the consumer of neutrino-storybook should be the one that owns the dependency, not neutrino-storybook.

That's why I advocate just removing all the dependencies on addons storybook altogether. The dev consuming neutrino-storybook is already going to have a clear preference for what they need for storybook, which is not likely to match what is provided in neutrino-storybook. Including storybook dependencies in neutrino-storybook is only likely to cause conflict rather than help.

The following package.json is what led to the yarn list --pattern 'storybook' above:

{
  "name": "neutrino-storybook-emotion",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "peerDependencies": {
    "prop-types": "^15",
    "react": "^16",
    "react-dom": "^16"
  },
  "scripts": {
    "start": "webpack-dev-server --mode development --open",
    "build": "webpack --mode production",
    "test": "jest",
    "lint": "eslint --cache --format codeframe --ext mjs,jsx,js src test",
    "storybook": "start-storybook -p 6006",
    "build-storybook": "build-storybook"
  },
  "devDependencies": {
    "@babel/core": "^7.13.10",
    "@neutrinojs/jest": "^9.5.0",
    "@neutrinojs/react-components": "^9.5.0",
    "@neutrinojs/standardjs": "^9.5.0",
    "@storybook/addon-actions": "^6.1.21",
    "@storybook/addon-essentials": "^6.1.21",
    "@storybook/addon-links": "^6.1.21",
    "@storybook/react": "^6.1.21",
    "babel-loader": "^8.2.2",
    "eslint": "^7",
    "jest": "^26",
    "neutrino": "^9.5.0",
    "neutrino-preset-storybook-react": "^1.0.2",
    "prop-types": "^15",
    "react": "^16",
    "react-dom": "^16",
    "webpack": "^4",
    "webpack-cli": "^3",
    "webpack-dev-server": "^3"
  },
  "resolutions": {
    "@storybook/react": "^6.1.21"
  },
  "dependencies": {}
}

This is what I would expect to see from yarn list --pattern "storybook":

$ yarn list --pattern "storybook"                                                   
yarn list v1.22.5
├─ @storybook/addon-actions@6.1.21
├─ @storybook/addon-backgrounds@6.1.21
├─ @storybook/addon-controls@6.1.21
├─ @storybook/addon-docs@6.1.21
├─ @storybook/addon-essentials@6.1.21
├─ @storybook/addon-links@6.1.21
├─ @storybook/addon-toolbars@6.1.21
├─ @storybook/addon-viewport@6.1.21
├─ @storybook/addons@6.1.21
├─ @storybook/api@6.1.21
├─ @storybook/channel-postmessage@6.1.21
├─ @storybook/channels@6.1.21
├─ @storybook/client-api@6.1.21
├─ @storybook/client-logger@6.1.21
├─ @storybook/components@6.1.21
├─ @storybook/core-events@6.1.21
├─ @storybook/core@6.1.21
├─ @storybook/csf@0.0.1
├─ @storybook/node-logger@6.1.21
├─ @storybook/postinstall@6.1.21
├─ @storybook/react@6.1.21
├─ @storybook/router@6.1.21
├─ @storybook/semver@7.3.2
├─ @storybook/source-loader@6.1.21
├─ @storybook/theming@6.1.21
├─ @storybook/ui@6.1.21
├─ neutrino-middleware-storybook@1.0.2
└─ neutrino-preset-storybook-react@1.0.2
constgen commented 3 years ago

I can see that you have installed "@storybook/react": "^6.1.21", and "neutrino-preset-storybook-react": "^1.0.2", at the same time which conflict. Because the main goal of neutrino-preset-storybook-react to not setup Storybook for React by youself. Also by definition the "preset" means that this is already some kind of opinionated configuration.

What I can recommend is to use more low level neutrino-middleware-storybook and build your own preset. You can look at the source code of neutrino-preset-storybook-react as an example.

Currently I am working on neutrino-preset-storybook-vue which will reuse the same neutrino-middleware-storybook. And there will be some enhancements in this middleware in the near future. So stay in touch. May be I will think about "too many addons". But there is a reason to keep addon-docs as it is quite tricky to configure with Neutrino

Tell me if it helped you and this is what you are looking for

jayliu50 commented 3 years ago

Thank you for your reply and again, for your work on this.

I just think that it's a loss of flexibility for the end developer. If the developer chooses to include certain add-ons (which I find to be very easy in Storybook), this repo could support their choice and do those difficult things as you mention, but not require add-ons on their behalf. If the repo declares dependencies on the developer's behalf, I see that as overstepping the boundary and results in tight coupling between this repo and whatever add-ons you choose. If you could support add-ons but not depend on them, I think this repo could be that much more powerful.

The pull request #2 is probably not perfect code but illustrates what I have in mind.

Maybe it's not in your vision to do what I'm suggesting and that's okay! Again, thank you for your public repo.

On Mon, Mar 22, 2021, 7:03 PM Constantine Genchevsky < @.***> wrote:

I can see that you have installed @.***/react": "^6.1.21", and "neutrino-preset-storybook-react": "^1.0.2", at the same time which conflict. Because the main goal of neutrino-preset-storybook-react to not setup Storybook for React by youself. Also by definition the "preset" means that this is already some kind of opinionated configuration.

What I can recommend is to use more low level neutrino-middleware-storybook https://github.com/constgen/neutrino-storybook/tree/master/packages/middleware-storybook and build your own preset. You can look at the source code of neutrino-preset-storybook-react as an example.

Currently I am working on neutrino-preset-storybook-vue which will reuse the same neutrino-middleware-storybook. And there will be some enhancements in this middleware in the near future. So stay in touch. May be I will think about "too many addons". But there is a reason to keep addon-docs as it is quite tricky to configure with Neutrino

Tell me if it helped you and this is what you are looking for

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/constgen/neutrino-storybook/issues/1#issuecomment-804452160, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFEQRRKYB7JZSB3YUCB3QDTE7EFZANCNFSM4ZEIQHYQ .