search

storybookjs / storybook

Storybook is the industry standard workshop for building, documenting, and testing UI components in isolation
https://storybook.js.org
MIT License
84.21k stars 9.26k forks source link

Lerna: Dependency cycles in devDependencies #6523

Closed gaetanmaisse closed 5 years ago

gaetanmaisse commented 5 years ago

Storybook monorepo contains some cyclic dependencies, they are listed during yarn bootstrap: 

...
storybook info bootstrap prepare
lerna notice cli v3.13.1
lerna WARN ECYCLE Dependency cycles detected, you should fix these!
lerna WARN ECYCLE @storybook/addon-a11y -> @storybook/components -> @storybook/addon-actions -> @storybook/components
lerna WARN ECYCLE @storybook/addon-actions -> @storybook/components -> @storybook/addon-actions
lerna WARN ECYCLE @storybook/addon-backgrounds -> @storybook/components -> @storybook/addon-actions -> @storybook/components
lerna WARN ECYCLE @storybook/addon-cssresources -> @storybook/components -> @storybook/addon-actions -> @storybook/components
lerna WARN ECYCLE @storybook/addon-info -> @storybook/components -> @storybook/addon-actions -> @storybook/components
lerna WARN ECYCLE @storybook/addon-jest -> @storybook/components -> @storybook/addon-actions -> @storybook/components
lerna WARN ECYCLE @storybook/addon-knobs -> @storybook/components -> @storybook/addon-actions -> @storybook/components
lerna WARN ECYCLE @storybook/addon-notes -> @storybook/components -> @storybook/addon-actions -> @storybook/components
lerna WARN ECYCLE @storybook/addon-storysource -> @storybook/components -> @storybook/addon-actions -> @storybook/components
lerna WARN ECYCLE @storybook/addon-viewport -> @storybook/components -> @storybook/addon-actions -> @storybook/components
lerna WARN ECYCLE @storybook/ui -> @storybook/components -> @storybook/addon-actions -> @storybook/components
lerna WARN ECYCLE @storybook/angular -> @storybook/core -> @storybook/ui -> @storybook/components -> @storybook/react -> 
lerna WARN ECYCLE @storybook/ember -> @storybook/core -> @storybook/ui -> @storybook/components -> @storybook/react -> 
lerna WARN ECYCLE @storybook/html -> @storybook/core -> @storybook/ui -> @storybook/components -> @storybook/react -> 
lerna WARN ECYCLE @storybook/marko -> @storybook/core -> @storybook/ui -> @storybook/components -> @storybook/react -> 
lerna WARN ECYCLE @storybook/mithril -> @storybook/core -> @storybook/ui -> @storybook/components -> @storybook/react -> 
lerna WARN ECYCLE @storybook/polymer -> @storybook/core -> @storybook/ui -> @storybook/components -> @storybook/react -> 
lerna WARN ECYCLE @storybook/preact -> @storybook/core -> @storybook/ui -> @storybook/components -> @storybook/react -> 
lerna WARN ECYCLE @storybook/react -> @storybook/components -> @storybook/ui -> @storybook/core -> @storybook/react
lerna WARN ECYCLE @storybook/riot -> @storybook/core -> @storybook/ui -> @storybook/components -> @storybook/react -> 
lerna WARN ECYCLE @storybook/svelte -> @storybook/core -> @storybook/ui -> @storybook/components -> @storybook/react -> 
lerna WARN ECYCLE @storybook/vue -> @storybook/core -> @storybook/ui -> @storybook/components -> @storybook/react -> 
lerna WARN ECYCLE @storybook/components -> @storybook/addon-actions -> @storybook/components
lerna WARN ECYCLE @storybook/core -> @storybook/react -> @storybook/components -> @storybook/ui -> @storybook/core
lerna WARN ECYCLE @storybook/addon-a11y -> @storybook/components -> @storybook/react -> @storybook/core -> @storybook/ui -> 
lerna WARN ECYCLE @storybook/addon-actions -> @storybook/components -> @storybook/react -> @storybook/core -> @storybook/ui -> 
lerna WARN ECYCLE @storybook/addon-backgrounds -> @storybook/components -> @storybook/react -> @storybook/core -> @storybook/ui -> 
lerna WARN ECYCLE @storybook/addon-cssresources -> @storybook/components -> @storybook/react -> @storybook/core -> @storybook/ui -> 
lerna WARN ECYCLE @storybook/addon-info -> @storybook/components -> @storybook/react -> @storybook/core -> @storybook/ui -> 
lerna WARN ECYCLE @storybook/addon-jest -> @storybook/components -> @storybook/react -> @storybook/core -> @storybook/ui -> 
lerna WARN ECYCLE @storybook/addon-knobs -> @storybook/components -> @storybook/react -> @storybook/core -> @storybook/ui -> 
lerna WARN ECYCLE @storybook/addon-notes -> @storybook/components -> @storybook/react -> @storybook/core -> @storybook/ui -> 
lerna WARN ECYCLE @storybook/addon-storysource -> @storybook/components -> @storybook/react -> @storybook/core -> @storybook/ui -> 
lerna WARN ECYCLE @storybook/addon-viewport -> @storybook/components -> @storybook/react -> @storybook/core -> @storybook/ui -> 
lerna WARN ECYCLE @storybook/ui -> @storybook/core -> @storybook/react -> @storybook/components -> @storybook/ui
...

These cyclic dependencies caused some build to fails because lerna doesn't know which libs should be built first. It was not a big deal when SB was fully JS but since we have some libs in TS type checking during compilation is failing, causing the full build to fail too. For instance, this build linked to https://github.com/storybooks/storybook/pull/6500.

In fact, there are cyclic dependencies mainly because @storybook/components has @storybook/addon-actions and @storybook/react as devDependencies (and the latter finally depends on @storybook/components) because we have stories for almost all components from @storybook/components. I'm a big fan of having stories in the same folder as the components they describe but it looks like it's causing some trouble in our case...

As more and more parts of SB will be migrated to TS I think we should discuss and try to find solutions about this.

--

🤷‍♂️ Not sure which tags I should put on this issue

leoyli commented 5 years ago

Perhaps there are some deeper issues. I think we should make our typing resilient and less duplicated. I personally not encourage inline typing and more favour using type for function and interface for class (well, maybe just using type to leverage the union typing in TS). And in most of time, using type inferences and reduce the need to type everything in great details.

Most of file will be modularized, which makes typing also modularized and potentially cyclic referenced (but we only need to get the type...). That is how I see why migrate to TS get weird errors. Why not just have a file just for typing?! Maybe less intuitive/convenient but I think the we should not mix type mingle with its implementation.

Here is my approach in addon-contexts. I basically separate the implementation and typing. And by doing so, I have been able to think in abstraction, reduced the noise at the same time.

ndelangen commented 5 years ago

I think we DO NOT have any production cyclic dependencies. it's devDependencies that make things cyclical.

The solution is likely to not include the devDependencies in the package.json of @storybook/components.

Because they are guaranteed to be there when developing in the monorepo.

Essentially all devDependencies could be in the package.json in the root, and our own packages don't need to appear in package.json if they are ONLY used during dev on that package.

gaetanmaisse commented 5 years ago

@ndelangen Yes exactly it's only due to devDependencies!

The solution is likely to not include the devDependencies in the package.json of @storybook/components.

I'm ok with that and already tried it 😉 by doing so it raises ts-lint error no-implicit-dependencies. BUT I just found that we can, fortunately, add a whitelist of modules that will be skipped during ts-lint check.

So if you're ok, I will add a commit to https://github.com/storybooks/storybook/pull/6500 to remove @storybook/addon-actions and @storybook/react from devDependencies of @storybook/components and add them to tslint whitelist.

ndelangen commented 5 years ago

Sounds good!

gaetanmaisse commented 5 years ago

Fixed with https://github.com/storybooks/storybook/pull/6500