atanasster / storybook-addon-deps

A storybook addon to add a dependencies tree exporer tab.
https://atanasster.github.io/storybook-addon-deps/?path=/docs/design-system-avatarlist--short
MIT License
58 stars 3 forks source link

Dependency blocks shows everything as a dependency of one component #12

Open alasdair009 opened 3 years ago

alasdair009 commented 3 years ago

When running the latest storybook 6.1.11 with storybook-addon-deps@2.1.3 the dependency block in the docs page shows all components as a dependency of a seemingly random component and none of the actual dependencies are displayed.

Code snippets for reproduction:

main.js

module.exports = {
    stories: ["../src/**/*.stories.mdx", "../src/**/*.stories.tsx"],
    presets: [
    ],
    addons: [
        "@storybook/addon-essentials",
        "@storybook/addon-a11y",
        "@storybook/addon-links",
        "@storybook/addon-storysource",
        "storybook-addon-deps"
    ]
};

preview.js

addParameters({
    options: {
        storySort: {
            order: ['Design System', 'Styles', 'Atoms', 'Molecules', 'Organisms', 'Templates'],
        },
        theme: theme,
    },
    docs: {
        page: DocsPage,
    },
    dependencies: {
      //display only dependencies/dependents that have a story in storybook
      //by default this is false
      withStoriesOnly: true,
      //completely hide a dependency/dependents block if it has no elements
      //by default this is false
      hideEmpty: true,
    },
    layout: 'padded',
    viewMode: 'docs',
    viewport: {
        viewports: {
            ...customViewports,
            ...INITIAL_VIEWPORTS
        },
    },
    withA11y: {}
});

export const decorators = [
    withThemeProvider,
    withDesign
];

GradientHover.tsx (this is the component that all others are shown have a dependency on)

export function GradientHover({ colorCentre, colorOuter, csx, children }: GradientHoverProps) {
    const [gradientX, setGradientX] = useState(50);
    const [gradientY, setGradientY] = useState(50);

    let width = 0;
    let height = 0;

    const setDimensions = (element: HTMLDivElement | null): void => {
        if (element) {
            const boundingBox = element.getBoundingClientRect();
            width = boundingBox.width;
            height = boundingBox.height;
        }
    };

    const handleMouseMove = (ev: MouseEvent<HTMLDivElement>): void => {
        const offsetX = ev.nativeEvent.offsetX;
        const offsetY = ev.nativeEvent.offsetY;

        setGradientX(offsetX / width * 100);
        setGradientY(offsetY / height * 100);
    };

    return (
        <Box
            data-testid={TEST_ID_GRADIENT_HOVER}
            onMouseMove={(event) => handleMouseMove(event)}
            ref={(element) => setDimensions(element)}
            sx={{
                alignItems: "center",
                display: "flex",
                background: `radial-gradient(circle at ${gradientX}% ${gradientY}%, ${colorCentre}, ${colorOuter})`,
                height: "100%",
                justifyContent: "center",
                left: 0,
                position: "absolute",
                top: 0,
                width: "100%",
                ...csx
            }}>{children}</Box>
    );
}

Close.tsx (a standard component in the library

export function Close({ onClick, csx, title }: CloseProps) {
    return (
        <IconButton icon={closeIcon} onClick={onClick} title={title} csx={csx} />
    );
}
alasdair009 commented 3 years ago

deps-bug Screenshot for reference of the docs page of the component

atanasster commented 3 years ago

Hi @alasdair009 - When did this start happening? do you have a repo that I can check?

alasdair009 commented 3 years ago

Hi @alasdair009 - When did this start happening? do you have a repo that I can check?

So we have not been able to pinpoint when but it happened post an upgrade to Storybook 6 around July.

In terms of a repro I wish I could share our full source but it’s under NDA. But I’ll try and spin up another one using the above config and some simple components. We have fully removed the add on and reinstalled it only to hit the same issue so it’s really got us stuck :)

atanasster commented 3 years ago

Thanks @alasdair009 pls let me know when you have a sample

kkckkc commented 3 years ago

I have a very similar issue that I've tracked down to the following construct. I'll see if I can create a sample repo

  1. I have module called @core-components with an index.js with the following content
    export { default as AccountMenu } from './AccountMenu/AccountMenu';
    export { default as AutocompletePostcode } from './AutocompletePostcode/AutocompletePostcode';
    export { default as BaseButton } from './BaseButton/BaseButton';
    export { default as ButtonGroup } from './ButtonGroup/ButtonGroup';
  2. In other parts of the code I have imports such as
    import { BaseButton } from '@core-components';

The import in step 2, associates the name BaseButton with the whole of index.js. This means that BaseButton is listed as a dependent of all components in index.js, like ButtonGroup and AccountMenu

A similar issue occurs if a component, like ButtonGroup does something like

import { BaseButton } from '..'

It seems to me the logic to determine dependencies when using import { abc } from 'xyz' is not working the way I expect it to.

alasdair009 commented 3 years ago

@kkckkc - so I removed my index.ts file that essentially exports all the components to allow them to be imported into other projects from the route of my SB project (rather than the full path).

export * from "./components/atoms/Button/Button";
export * from "./components/atoms/Checkbox/Checkbox";
export * from "./components/atoms/Chevron/Chevron";
export * from "./components/atoms/Close/Close";

That appears to sort it out. But it's not ideal - but that appears to be the reproduction. Nice one!

kkckkc commented 3 years ago

I've created a sample repo at https://github.com/kkckkc/storybook-addon-deps-issue to illustrate the problem

image image

Two issues here;

  1. The RedGray component should list Red and Gray as dependencies - not Blue as is listed now
  2. The Gray component should list RedGray as dependents - not Blue as is listed now
atanasster commented 3 years ago

@kkckkc thank you very much i will post here as soon as i find out what is going on

kkckkc commented 3 years ago

I've created a small change in the following fork https://github.com/kkckkc/storybook-dep-webpack-plugin that seems to fix the issue in the sample repo.

The idea is to add heuristics to identify these module directory files by using a naming pattern and checking that it has a named export that corresponds to an import with the same name

atanasster commented 3 years ago

That’s great - do you want to submit a PR?

atanasster commented 3 years ago

Thanks @kkckkc - just published 1.0.6 with your PR, can you guys give it a try

kkckkc commented 3 years ago

It works perfectly fine with the test repo - however, I'm still seeing some issues with some of my more complex and larger repos. I will try to track down the issue and submit another PR

alasdair009 commented 3 years ago

So this could be my naivety here but when I add: "storybook-dep-webpack-plugin": "^1.0.6",

to the package.json and then run a build I get the following error:

.../node_modules/storybook-dep-webpack-plugin/lib/index.js:109
const contextPath = this.shortenFolder(effectiveDep.module.context);

TypeError: Cannot read property 'context' of null
kkckkc commented 3 years ago

Yes, it seems, depending on your exact setup, there is a situation in which module is undefined. It wasn't found in the test repo, but I've seen it in some of my larger code bases as well.

This PR should fix the issue https://github.com/atanasster/storybook-dep-webpack-plugin/pull/5

If you want to try immediately, you can manually backport the PR to node_modules/storybook-dep-webpack-plugin/lib/index.js and see if it solves the issue for you as well

atanasster commented 3 years ago

thanks guys, just pushed 1.0.7 with the latest from @kkckkc

alasdair009 commented 3 years ago

thanks guys, just pushed 1.0.7 with the latest from @kkckkc

So I built against this and whilst it fixed the compile error the main issue is still present (ie everything is still a dependent of one component)

atanasster commented 3 years ago

Thanks for testing it @alasdair009 - did you have a repo to reproduce? I am on the road for one more week and can take a deeper look then - but in the meantime if @kkckkc has some more ideas?

kkckkc commented 3 years ago

Yes, I will try to have a look and see if I can reproduce in a repo. The thing I want to check is if the TypeScript import/export constructs ends up with the same dependency structure in webpack as for the JavaScript example.

The other thing that I think would be useful debugging these scenarios, is to add an option to dump the assets object for debugging purposes.

kkckkc commented 3 years ago

Can you try to set compilerOptions.module to ESNext in your tsconfig.json

If it's set to commonjs for instance (which I think is the default), there is no way to look at something like

import { xyz } from 'module'

and "get access to" the xyz name. instead the typescript transpilation erases the name and the result is essentially that you import the full module. This leads to the issue of incorrect dependents as discussed above.

See https://www.typescriptlang.org/tsconfig#module for how the various module options are transpiled.

alasdair009 commented 3 years ago

Can you try to set compilerOptions.module to ESNext in your tsconfig.json

Set the tsconfig as follows (along with dep-webpack to 1.0.7)...

{
    "compileOnSave": false,
    "compilerOptions": {
        "target": "es5",
        "allowJs": true,
        "skipLibCheck": true,
        "strict": true,
        "esModuleInterop": true,
        "moduleResolution": "node",
        "module": "ESNext",
        "jsx": "react",
        "allowSyntheticDefaultImports": true,
        "noImplicitReturns": true,
        "noUnusedLocals": true,
        "outDir": "./dist/lib",
        "baseUrl": "./src/",
        "declaration": true,
        "forceConsistentCasingInFileNames": true
    },
    "include": ["**/*.ts", "**/*.tsx"],
    "exclude": [
        "node_modules",
        "dist",
        "tests",
        "**/*.test.ts",
        "**/*.test.tsx",
        "dev-scripts"
    ]
}

Issue remains - I'll try and find sometime this week to setup a simpler repo to show you guys (the current project I'm working on is not suitable for making public) :)

kkckkc commented 3 years ago

Ok, I can sort of replicate your issue

At least in my simple sample repo I can work around it, by replacing (in your case)

export * from "./components/atoms/Button/Button";
export * from "./components/atoms/Checkbox/Checkbox";
export * from "./components/atoms/Chevron/Chevron";
export * from "./components/atoms/Close/Close";

with

export { Button } from "./components/atoms/Button/Button";
export { Checkbox } from "./components/atoms/Checkbox/Checkbox";
export { Chevron } from "./components/atoms/Chevron/Chevron";
export { Close } from "./components/atoms/Close/Close";

I will try to see if this can be fixed in the webpack plugin somehow

alasdair009 commented 3 years ago

@kkckkc - yep that's it. 😄 😄 😄 If I make your change as suggested it works.

And in some ways I feel that is actually better but that still feels like a bug that needs resolving.

kkckkc commented 3 years ago

Yes, I agree it's a defect. I have some sort of solution at https://github.com/kkckkc/storybook-dep-webpack-plugin/commit/6505fc2b0036f7bb4369226888c16753171dfa67 if you want to try - however, I don't fully understand how this actually works, so I'm not comfortable in submitting a PR right now. I think there needs to be some more testing done for the various imports of imports and exports.